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

CSS3DSprite with parent scale #22235

Merged
merged 5 commits into from
Aug 20, 2021
Merged

CSS3DSprite with parent scale #22235

merged 5 commits into from
Aug 20, 2021

Conversation

giusepperaso
Copy link
Contributor

CSS3DSprite seems to ignore parent scale completely; this seems to fix it, but I don't know if there are better / cleaner solutions

CSS3DSprite seems to ignore parent scale completely; this seems to fix it, but I don't know if there are better / cleaner solutions
@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 1, 2021

Also would be nice to have a live example that demonstrates the issue. Such an example can then be used to verify your fix.

use the world matrix of the same object to fix scale
@giusepperaso
Copy link
Contributor Author

This is a very basic example of CSS3DSprite not taking the parent scale
https://codesandbox.io/s/ancient-rgb-21j7h?file=/src/index.js

I have adjusted the code to just call getWorldScale of the object itself, which internally uses decompose

Put a target for getWorldMatrix
Vectors are declared at the top and reused; getWorldScale is replaced by direct use of decompose in order to reuse vectors
_matrix.copyPosition( object.matrixWorld);

object.matrixWorld.decompose( _vector, _quaternion, _scale );
_matrix.scale( _scale )
Copy link
Collaborator

@Mugen87 Mugen87 Aug 2, 2021

Choose a reason for hiding this comment

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

Please use this approach:

object.matrixWorld.decompose( _position, _quaternion, _scale );
_matrix.setPosition( _position );
_matrix.scale( _scale );

Copy link
Collaborator

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.

Copy link
Contributor Author

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)

Copy link
Collaborator

@WestLangley WestLangley Aug 4, 2021

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.

Copy link
Contributor Author

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

Copy link
Collaborator

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.

const object = new CSS3DObject( element2 );
object.position.set( 0, 0, 0 );
object.rotation.set( 0, 0, 0 );
object.scale.set( 1, 1, 1 );

const sprite = new CSS3DSprite( element1 );
sprite.position.set( 0, 0, 0 );
sprite.rotation.set( 0, 0, 0 );
sprite.scale.set( 1, 1, 1 );
sprite.rotation2D = 0;

const spriteChild = new CSS3DSprite( element3 );
spriteChild.position.set( 0, 0, 0 );
spriteChild.rotation.set( 0, 0, 0 );
spriteChild.scale.set( 1, 1, 1 );
spriteChild.rotation2D = 0;

const group = new THREE.Group();
group.position.set( 0, 0, 0 );
group.rotation.set( 0, 0, 0 );
group.scale.set( 1, 1, 1 );

group.add( object );
group.add( sprite ) ;
sprite.add( spriteChild );
//group.add( spriteChild );

Copy link
Contributor Author

@giusepperaso giusepperaso Aug 4, 2021

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

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.

@mrdoob mrdoob added this to the r132 milestone Aug 3, 2021
call setPosition with the result from decompose
@mrdoob
Copy link
Owner

mrdoob commented Aug 10, 2021

Is this PR ready to go?

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 20, 2021

@mrdoob This PR can be merged 👍 .

@mrdoob mrdoob merged commit 686e5de into mrdoob:dev Aug 20, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 20, 2021

Thanks!

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