Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Dec 28, 2018

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Dec 28, 2018

I think I have originally suggested SortingGroup in order to avoid any semantic changes to Group. If that's not a problem, we can avoid to introduce a new class.

if ( object.isLight ) {
if ( object.isGroup ) {

groupOrder = object.renderOrder;
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are passing in groupOrder and reassigning the value if it is a group???

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, it will apply to its children only.

var material = object.material;

currentRenderList.push( object, geometry, material, _vector3.z, null );
currentRenderList.push( object, geometry, material, groupOrder, _vector3.z, null );
Copy link
Collaborator

Choose a reason for hiding this comment

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

You are passing in groupOrder and setting the last arg to null because it is not a group???

Copy link
Owner

Choose a reason for hiding this comment

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

The last argument is for multi-materials.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. Group vs. groups.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, sorry. Group vs material group.

@mrdoob mrdoob added this to the r100 milestone Dec 28, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jan 2, 2019

Let's see how this goes 🤞

@mrdoob mrdoob merged commit e5aa0f2 into mrdoob:dev Jan 2, 2019
@mrdoob
Copy link
Owner

mrdoob commented Jan 2, 2019

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Jan 2, 2019

Fixes #14415?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 2, 2019

Yes! 🎉

@WestLangley
Copy link
Collaborator

When this property is set for an instance of [page:Group Group], all descendants objects will be sorted and rendered together.

How can a group not have the value set? Zero is a valid value, correct?

So does that mean that objects within a group are always rendered together, and the render order applies to only the objects within the group?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Jan 2, 2019

How can a group not have the value set? Zero is a valid value, correct?

Well, I've meant if a value other than the default value is set. Feel free to improve the docs.

So does that mean that objects within a group are always rendered together, and the render order applies to only the objects within the group?

Yes, relative sorting should be possible.

@WestLangley
Copy link
Collaborator

Feel free to improve the docs.

I can't, because I do not understand what the intended behavior is. Perhaps you can compose a clear statement that describes the intended behavior.

Also, you have modified the sorting behavior for everyone to accommodate an edge case. Based on my current understanding, I think adding SortingGroup would have been a better option.

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.

3 participants