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

Ensure a-entity rotation Euler has YXZ order even without rotation component #5513

Merged
merged 1 commit into from Apr 8, 2024

Conversation

mrxz
Copy link
Contributor

@mrxz mrxz commented Apr 8, 2024

Description:
When using the rotation component the order is YXZ as set by the component. When setting rotation programmatically the recommended approach is directly through Object3D.rotation (docs). It's implied that the following two behave the same:

// With three.js
el.object3D.rotation.set(
  THREE.MathUtils.degToRad(15),
  THREE.MathUtils.degToRad(30),
  THREE.MathUtils.degToRad(90)
);

// With .setAttribute (less recommended).
el.setAttribute('rotation', {x: 15, y: 30, z: 90});

However, this is only the case if the element has had a rotation component before. Otherwise the user would have to explicitly set or pass the order. For consistency this PR sets the euler order upon entity creation, avoiding the issue.
Example Glitch showing the inconsistency: https://glitch.com/edit/#!/mud-grateful-haze

Also noticed a small optimization opportunity. The rotation component sets the order separately, which actually invokes a setter that triggers a recalculation of the corresponding Object3D.quaternion. The Euler.set method actually allows the order to be passed as well, preventing this redundant calculation.

Changes proposed:

  • Set rotation euler order upon entity creation
  • Pass euler order into set method avoiding redundant calculations

@dmarcos
Copy link
Member

dmarcos commented Apr 8, 2024

Thanks so much!

@dmarcos dmarcos merged commit 763b3fe into aframevr:master Apr 8, 2024
1 check passed
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.

None yet

2 participants