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

CSS2DRenderer: Round values used for translation. #21416

Merged
merged 3 commits into from
Mar 18, 2021

Conversation

simondate
Copy link
Contributor

@simondate simondate commented Mar 5, 2021

Related issue: Fixed #21415

Description

Uses Math.round to set only whole numbers for values being used by CSS2DRenderer

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 5, 2021

Do you mind running node utils/modularize.js from the project's root directory to generate the ES6 module of CSS3DRenderer?

@Mugen87 Mugen87 changed the title Fixes #21415 CSS2DRenderer: Round values used for translation. Mar 5, 2021
@mrdoob mrdoob added this to the r127 milestone Mar 5, 2021
@simondate

This comment has been minimized.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 9, 2021

That looks right. You should see with git that examples/jsm/renderers/CSS2DRenderer.js has changed.

@simondate
Copy link
Contributor Author

@Mugen87 sorry forgot to push!

Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

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

Even if this PR fixes your problem, it would be good if you could report it to WebKit: https://bugs.webkit.org/

If WebKit would fix the issue, this change would not be required and could be reverted.

@mrdoob
Copy link
Owner

mrdoob commented Mar 9, 2021

@elalish I think you're one of the few users of CSS2DRenderer. Do you know if this change will be noticeable?

@mrdoob
Copy link
Owner

mrdoob commented Mar 12, 2021

Considering this only affects Safari I think it'd be better to do this:

if ( /apple/i.test( navigator.vendor ) ) {

	// https://github.com/mrdoob/three.js/issues/21415
	element.style.transform = 'translate(-50%,-50%) translate(' + Math.round( vector.x * _widthHalf + _widthHalf ) + 'px,' + Math.round( - vector.y * _heightHalf + _heightHalf ) + 'px)';

} else {

	element.style.transform = 'translate(-50%,-50%) translate(' + ( vector.x * _widthHalf + _widthHalf ) + 'px,' + ( - vector.y * _heightHalf + _heightHalf ) + 'px)';

}

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 16, 2021

@simondate Do you mind updating the PR with @mrdoob's suggestion?

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 18, 2021

Now it depends whether we want to risk this change or not, see #21415 (comment).

@mrdoob
Copy link
Owner

mrdoob commented Mar 18, 2021

I don't see much risk... Labels movement won't be as smooth in Safari but that's it I think.

@mrdoob mrdoob merged commit a87f87b into mrdoob:dev Mar 18, 2021
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.

CSS2DObject - potential rendering issue in Safari MacOS
3 participants