Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add geo command for cylindrical billboarding #856

Open
wants to merge 8 commits into
base: develop/2.4.0
Choose a base branch
from

Conversation

LucretiaArc
Copy link

Requested by Arctic

@Reonu
Copy link
Contributor

Reonu commented Jan 21, 2025

Hell yeah

Copy link
Collaborator

@gheskett gheskett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please repoint this to HackerSM64 2.4, since this isn't going to introduce any major conflicts (although it will conflict minorly with Lighting Engine).

include/geo_commands.h Outdated Show resolved Hide resolved
include/geo_commands.h Outdated Show resolved Hide resolved
@LucretiaArc LucretiaArc changed the base branch from develop/3.0.0 to develop/2.4.0 January 21, 2025 13:31
@LucretiaArc LucretiaArc changed the base branch from develop/2.4.0 to develop/3.0.0 January 21, 2025 13:31
@gheskett gheskett changed the base branch from develop/3.0.0 to develop/2.4.0 January 21, 2025 13:47
Copy link
Collaborator

@gheskett gheskett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, assuming this was tested with newest changes and both with and without a DL.

@arthurtilly
Copy link
Collaborator

would it perhaps be possible to make this a parameter for the command rather than a new ID? (can create a compatibility macro with one fewer param for existing repos / fast64)

@gheskett
Copy link
Collaborator

If the code isn't too different, I'd agree with this honestly

@LucretiaArc
Copy link
Author

Done, node pos/direction to camera version is gone now too, but easy to readd if needed

src/engine/math_util.c Outdated Show resolved Hide resolved
include/geo_commands.h Outdated Show resolved Hide resolved
src/engine/math_util.c Outdated Show resolved Hide resolved
@arthurtilly
Copy link
Collaborator

looks better now but tbh i'm not totally sure how necessary axis is either

@LucretiaArc
Copy link
Author

The axis param is very helpful in cases where the axis you want to use isn't aligned to X, Y, or Z. For example, if someone adds a billboarded rope to their level, they can translate the node to one end of the rope, then set the axis to the relative position of the other end. If we force people to use a rotation node for this, they'll have to calculate the angle themselves.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants