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

CSS2DObject - potential rendering issue in Safari MacOS #21415

Closed
simondate opened this issue Mar 5, 2021 · 13 comments · Fixed by #21416
Closed

CSS2DObject - potential rendering issue in Safari MacOS #21415

simondate opened this issue Mar 5, 2021 · 13 comments · Fixed by #21416

Comments

@simondate
Copy link
Contributor

simondate commented Mar 5, 2021

Describe the bug

Our team has used Three.JS to create a Virtual Tour content type that allows content authors to create interactions which enables the user to navigate around a scene, click on interactions and change to different scenes.

We've recently updated our this content type to work with labels which are 2d objects. However after testing in Safari we started to notice that lines would be drawn all over the scene from our labels. These lines go away when you hover over a label again however they keep on coming back and look quite distracting for a user.

Potential fix

I found that by using whole numbers, rather than decimal numbers, for the pixel position of the buttons (which the labels that cause the issue are a child of). This would be done by adding Math.round twice on this line. I've made the change on our fork of the Three.JS library and it has has issue has been gone since.

I was wondering if anyone had noticed any issue like this and if this fix is valid? Or if it's instead caused by another change we made somewhere.

Code

https://github.com/h5p/h5p-three-image/tree/release
https://github.com/h5p/h5p-three-js
https://github.com/h5p/h5p-three-sixty/tree/release

Demo

https://staging.h5p.org/node/622030

Screenshot and video

https://www.screencast.com/t/1J7ZjSWc

Screenshot 2021-03-05 at 14 06 20

Platform:

  • Device: [Desktop]
  • OS: [MacOS]
  • Browser: [Safari]
  • Three.js version: [Not sure, but was updated Feb 2019]
@mrdoob
Copy link
Owner

mrdoob commented Mar 5, 2021

Seems like Safari invalidation code is buggy...

I'm curious to see what the side effects of adding this workaround are. Could you do a Pull Request with the change?

@mrdoob mrdoob added this to the rXXX milestone Mar 5, 2021
@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 5, 2021

Maybe https://bugs.webkit.org/show_bug.cgi?id=215590?

In this case the issue is a duplicate of #19854?

simondate added a commit to simondate/three.js that referenced this issue Mar 5, 2021
@simondate
Copy link
Contributor Author

@Mugen87 This issue only occurs with 2d objects, if 3d objects are used the problem goes away.

I've tested it both on my own Macbook as well as using BrowserStack with Safari and MacOS and got the same outcome. I couldn't replicate on Safari on my iPad.

@Mugen87
Copy link
Collaborator

Mugen87 commented Mar 5, 2021

It still seems that the problem with Safari is caused by the same issue. Meaning Safari is rounding during the translation step, see #20069 (comment).

@mrdoob
Copy link
Owner

mrdoob commented Mar 5, 2021

I don't think it's the same issue.
I think #20069 is about WebKit rounding numbers internally.
This issue is about WebKit not invalidating the drawn area correctly so it leaves trails.
Chrome used to have this issue years ago.

simondate added a commit to simondate/three.js that referenced this issue Mar 9, 2021
@elalish
Copy link
Contributor

elalish commented Mar 9, 2021

Hmm, I don't repro the bug on my macbook pro on Safari using this hotspots example: https://modelviewer.dev/examples/annotations/index.html#addHotspots

This uses the CSS2DRenderer, so maybe there's a different workaround? I'm a little concerned about rounding to integer pixels since there are now commonly several real pixels per CSS pixel. I think it could adversely affect smoothness of motion.

@simondate
Copy link
Contributor Author

@elalish I can't replicate in your demo either.

I'm not entirely sure if the fact it's a scene being rotated instead of an object that is being viewed that would make a difference?

@mrdoob mrdoob removed this from the rXXX milestone Mar 18, 2021
mrdoob pushed a commit that referenced this issue Mar 18, 2021
* Fixes #21415

* Built the modules for #21415

* Added check for Safari before using changed code
@wooster0
Copy link
Contributor

Apologies if I'm missing something but shouldn't this branch be removed now that this issue linked in the comment in this branch is fixed?

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

@mrdoob
Copy link
Owner

mrdoob commented Feb 24, 2022

Where does it say it has been fixed?

@anton-ax
Copy link

This fix creates a new problem, now labels is shaking in safary :(

@mrdoob
Copy link
Owner

mrdoob commented Apr 30, 2022

@Native1989 can you share a screen recording?

@fanciful-marmot
Copy link
Contributor

fanciful-marmot commented May 17, 2022

@mrdoob

Close up video of jitter from rounding on Safari:

safari-jitter.mov

Comparison on Chrome:

chrome-smooth.mov

Here's the codesandbox used in the videos: https://codesandbox.io/s/exciting-lederberg-f0uu78?file=/src/index.js
The red square is rendered with CSS2DRenderer.

I'd be happy to open a PR removing the rounding if it's not needed any more

@mrdoob
Copy link
Owner

mrdoob commented May 19, 2022

I'd be happy to open a PR removing the rounding if it's not needed any more

Yeah, lets try removing it 👍

fanciful-marmot added a commit to fanciful-marmot/three.js that referenced this issue May 24, 2022
mrdoob pushed a commit that referenced this issue May 25, 2022
* Remove position rounding in CSS2DRenderer

Relates to issue #21415

* Run build-examples
abernier pushed a commit to abernier/three.js that referenced this issue Sep 16, 2022
* Remove position rounding in CSS2DRenderer

Relates to issue mrdoob#21415

* Run build-examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants