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

CSS2D/3DRenderer: Make the code more reasonable. #28393

Merged
merged 1 commit into from
May 16, 2024
Merged

Conversation

lafflan
Copy link
Contributor

@lafflan lafflan commented May 16, 2024

IMO, i don't think it's necessary to traverse the entire scene every frame, only traverse and hide when object.visible === false is more reasonable.

@yomotsu
Copy link
Contributor

yomotsu commented May 16, 2024

As I mentioned before:
#28293 (comment)
I think ALL objects need to be hidden before rendering.

Therefore, this change might break the expected behavior.
Would you mind testing with nested CSS2D/3DObjects?

like
hidden object > visible object Should hide both objects. but the child may be rendered with this change.

@lafflan
Copy link
Contributor Author

lafflan commented May 16, 2024

like
hidden object > visible object Should hide both objects. but the child may be rendered with this change.

I have tested in examples/css2d_label.html of both nested mesh > css2d and css2d > css2d situation, all results appear to meet expectations.

@lafflan lafflan closed this May 16, 2024
@lafflan lafflan reopened this May 16, 2024
@mrdoob
Copy link
Owner

mrdoob commented May 16, 2024

@lafflan Any chance you can share the code you used for testing that?

@lafflan
Copy link
Contributor Author

lafflan commented May 16, 2024

@lafflan Any chance you can share the code you used for testing that?

OK. Test in https://jsfiddle.net/q8c6d2ym/6/

@yomotsu
Copy link
Contributor

yomotsu commented May 16, 2024

Sorry, I misunderstood.
Your PR is correct, since hideObject() works in the object tree (not DOM).

I also tested it with a simplified fork of your demo:
https://jsfiddle.net/dkyfct6o/
and it works too.

Very sorry.

@Mugen87 Mugen87 added this to the r165 milestone May 16, 2024
@Mugen87 Mugen87 merged commit de367cb into mrdoob:dev May 16, 2024
20 checks passed
@lafflan lafflan deleted the patch-1 branch May 17, 2024 00:40
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