-
-
Notifications
You must be signed in to change notification settings - Fork 36.1k
CSS3DSprite with parent scale #22235
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
39f550b
CSS3DSprite with parent scale
giusepperaso f5bbe0f
CSS3DSprite set scale from world matrix
giusepperaso 4a21004
Fix getWorldMatrix deprecation warning
giusepperaso 4d57ff4
Reusing Vectors and calling decompose directly
giusepperaso c504cd4
use decompose to also set position
giusepperaso File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use this approach:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this will work, but it needs to be fully-tested with a rotated parent object and/or a rotated child sprite -- in addition to all the scale possibilities. CSS3DSprites have a separate rotation property.
Use a very simple testbed. The one you provided was a good one. :-) You do not need to include the test case in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added setPosition and I have adjusted the example ( https://codesandbox.io/s/ancient-rgb-21j7h?file=/src/index.js ) to have a rotation on the parent group and on the sprite, with constants at the top
However, with the current version of sprite, it seems that no rotation is applied at all (I don't know if this is the desired effect)
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I wanted a full test case -- and also as a courtesy to @Mugen87.
Sprites always render right-side-up relative to the camera. This is why the matrix (most-likely) had to be decomposed and the rotation removed.
Instead, there is
sprite.rotation2D. It should work correctly when the parent is not rotated.When the parent is also a rotated
CSS3DSprite, it remains to be seen if this code works in an acceptable manner.It may help if you added axes as a frame-of-reference to your test case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have put axes inside the example (in a webglrenderer) and I have included a yellow sprite which is a child of the red sprite
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your example, but one can't conclude anything from it because it is too complex. Start with something similar to this, and change a few parameters at a time and see if the library with your changes is working as expected.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have setup another sandbox based on your suggestion
https://codesandbox.io/s/css3dsprite-patch-three-rx5ui?file=/src/index.js
The relevant part is in index, while the setup and the elements are in separated files
It's also possible to switch between the patched version and the original version with the flag USE_PATCH
I tested different scenarios and it seems good to me except for one thing: if you have a sprite child with a sprite parent and you rotate the sprite child, the scale of the sprite child seems to apply incorrectly. The current codesandbox shows this scenario. I am not sure if it is really a bug and/or if it is worth worrying
PS: I wanted to import the patch directly from my fork but Codesandbox fails to request it from Github, so I had to recreate it manually
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see evidence of that. Maybe someone else can have a look...
One must avoid setting the sprite matrix rotation, but that is not new.