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

Remove THREE.Group() and replace with THREE.Object3D(). #328

Merged
merged 1 commit into from
Aug 7, 2020

Conversation

J-Rojas
Copy link
Contributor

@J-Rojas J-Rojas commented Aug 6, 2020

Groups change the render sort order globally, so using these in model loading may cause unintended side-effects.

Groups change the render sort order globally, so using these in model loading may cause side-effects.
@J-Rojas J-Rojas requested a review from jihoonl August 6, 2020 19:31
@jihoonl
Copy link
Member

jihoonl commented Aug 7, 2020

It looks like original code still uses Group(). ColladaLoader.js OBJLoader.js

Would you PR on the main repo as well to check if it would cause side effect? This code is basically a copy from main repo with small changes for z-axis hack.

@J-Rojas
Copy link
Contributor Author

J-Rojas commented Aug 7, 2020

Sure, I can try, however those are just meant as examples, not production level code. They are slow to merge changes at the moment because they are doing a huge refactor.

More context: Group is identical to Object3D, except it creates a new condition for the render order in the scene. The render list is a globally sorted list and it prioritizes Groups first over Object3Ds. If you load a model and try to include it into a Group with a non-zero renderOrder, these internally generated Groups model will be rendered in ways that are unexpected due to the global render sorting. The groups here really bear no significance since they have renderOrder == 0 and they aren't intended to manage the render order in anyway, so this change avoids this potential side effect.

@J-Rojas
Copy link
Contributor Author

J-Rojas commented Aug 7, 2020

See this PR. This gentlemen would like to address the strangeness of the global rendering order. His PRs have been ignored.

mrdoob/three.js#19526

@jihoonl
Copy link
Member

jihoonl commented Aug 7, 2020

I see. Ok thanks for the explanation.

@J-Rojas J-Rojas merged commit 90ad89b into RobotWebTools:develop Aug 7, 2020
@J-Rojas J-Rojas deleted the remove-groups branch August 7, 2020 06:20
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.

2 participants