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

Correctly support global matrix in glTF interactivity #16285

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

RaananW
Copy link
Member

@RaananW RaananW commented Mar 12, 2025

Because of babylon's RHS->LHS conversion of any glTF scene, there was an issue with using the global matrix compared to local matrix in glTF interactivity (or more percise in the glTF json pointer system).

Local matrix is actually in RHS, as the node itself doesn't change the properties provided from glTF. However, globalMatrix was (incorrectly) in LHS. The solution is to provide the global matrix in RHS as well. I am doing it by inverting the root node's transformation and multiplying it. This way we support RHS (identity) and LHS (rotation/scaling combination).

This PR also changes the JSONPointer block and the getproperty block to be a cached operation, meaning calculations will only be done once per execusion. This will optimize the execusion.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@RaananW RaananW marked this pull request as draft March 12, 2025 14:15
@RaananW
Copy link
Member Author

RaananW commented Mar 12, 2025

Fully testing this ATM.

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Mar 12, 2025

@RaananW RaananW marked this pull request as ready for review March 12, 2025 18:24
@RaananW RaananW enabled auto-merge (squash) March 12, 2025 19:11
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.

3 participants