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

Layers: add recursive property #28427

Closed
wants to merge 13 commits into from

Conversation

CodyJasonBennett
Copy link
Contributor

@CodyJasonBennett CodyJasonBennett commented May 19, 2024

Related issue: #18937

Description

Continues #19012 with handling wherever a visibility test is performed. I did not update layers tests in render-lists or transmission pass code since it was not clear to me if those lists were in any hierarchical order.

When Layers.recursive = true for an object and it fails the visibility test, its children are not processed.

Copy link

github-actions bot commented May 19, 2024

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev Filesize PR Diff
678.5 kB (168.2 kB) 678.8 kB (168.3 kB) +307 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev Filesize PR Diff
456.7 kB (110.3 kB) 456.9 kB (110.4 kB) +271 B

@Mugen87 Mugen87 added this to the r165 milestone May 21, 2024
@mrdoob mrdoob modified the milestones: r165, r166 May 31, 2024
@@ -3,6 +3,7 @@ class Layers {
constructor() {

this.mask = 1 | 0;
this.recursive = false;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the default should be configurable with a static property Layers.DEFAULT_RECURSIVE?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm personally not a fan of DEFAULT_ static properties since one could argue to introduce them for all normal properties to configure their default values. That makes the code more verbose and unnecessary complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the property has to be particularly notable and value contentious to be worth reconfiguring.

Ideally, this would work in user-land for cases like #18937 (comment):

THREE.Layers.prototype.recursive = true;

console.log( new THREE.Layers().recursive ) // false

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that this argumentation could be done for any other property as well. It's best if @mrdoob makes a design decision here. I'm not sure how he feels about the DEFAULT_ properties and to what extend they should be used.

The intended code should also look like so since ideally devs do not access the prototype object:

const layers = new THREE.Layers();
layers.recursive = true;

I understand you have to set recursive to true for each instance but that doesn't seem like a real burden to me, tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not every property has a highly contentious default value, so I have to disagree here on making this a general argument against static properties. This was my compromise for #18937 since I see a lot of resistance against changing the default to true, but if we can't allow this by configuration, then I propose making this the default instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to #19012 (comment), we want to keep false as the default.

This is also my expected behavior of layers, tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to put three users on the happy path and find compromises which don't lead them into footguns undermining performance or ease-of-use, and I'm especially frustrated to fight tooth and nail to advocate for them. That entire thread is proof of disagreement around a default value, which suggests to me this should be configurable. Setting layers per object is only error-prone for their case, and this discussion alone infinitely more energy than adding a static member.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this isn't even satisfactory for @gkjohnson, then I can simply retract this PR. I'm not aware of how this is handled in user-land today.

Copy link
Collaborator

@Mugen87 Mugen87 Jun 4, 2024

Choose a reason for hiding this comment

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

TBH, I never use layers recursively and if so I would probably traverse through the object hierarchy once and copy the layers to all objects that require the same mask. Granted a property that makes things configurable is easier to use but the PR also proves an increase in complexity in all components checking layers.

I'm not sure how common it is to user layers recursively in order to justify this complexity. I'm also not aware how common it is to mix recursive layer usage with non-recursive usage. If this is more a global setting then I agree a static DEFAULT_RECURSIVE makes sense.

However, I would really like to hear @mrdoob's opinion on this change and also how he feels about the additional checks in all renderers and raycaster.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this isn't even satisfactory for @gkjohnson,

I think I'm too far removed from that PR since it was over 4 year ago, now, and haven't been using layers recently. I'll have to leave this to others.

@CodyJasonBennett CodyJasonBennett marked this pull request as draft June 6, 2024 04:58
@CodyJasonBennett
Copy link
Contributor Author

Let's revisit this when there's a clear need for it or we find a better balance with complexity.

@CodyJasonBennett CodyJasonBennett deleted the recursive-layers branch June 8, 2024 10:19
@yfunk
Copy link

yfunk commented Jun 9, 2024

Thanks for continuing the work on this feature. I've been looking forward to recursive layers in Three.js for a while now, so it's sad to see progress on this halted once again.

Since it seems that this PR was closed in part due to the lack of a clear use case for the feature, I thought I would share mine.

Our team is developing a 3D editor application where we use the postprocessing library to render a single Three.js Scene in multiple render passes and selectively apply effects to different parts of the scene. For example, we only want to apply tone mapping and color grading to the main 3D model, not to viewport gizmos like TransformControls.

Currently, we achieve this by organizing all objects in the scene under different root nodes (e.g., modelRoot, gizmosRoot). When rendering a single frame, before each render pass, we adjust the visibility property of every root node so that only the relevant ones are rendered in that pass. However, this quickly gets complex to manage when you consider that the visibility of a root node can also change due to various external factors (e.g. the user wanting to hide all gizmos).

Layers would be a much better fit for such a use case. But with the current implementation, there are two significant disadvantages to this approach:

  • Layers don't apply recursively to children (like visibility does), so we have to make sure that every single object in the scene has the correct layer assigned to it at all times.
  • A node that is not on an active layer does not stop scene traversal during render. This results in noticeably worse overall performance for larger scenes, since the renderer now has to traverse the entire scene for every render pass in a single frame (instead of stopping at the hidden root nodes when using visibility)

Recursive layers would solve both issues and make it much easier to efficiently implement such a multi-pass rendering pipeline.

Regarding the discussion on whether the default value of the proposed recursive property should be configurable: This particular use case doesn't necessarily require it, as the value can be set alongside the layer number when creating the root node. However, if there is a use case that benefits from layers being recursive by default, personally, I see no problem with adding a static property as has been done before.

@CodyJasonBennett
Copy link
Contributor Author

I think I simply don't have the energy to champion this. I've used these changes multiple times in production for a similar use-case, where a certain layer is rendered in post-processing. I found that collateral matrix updates were quite large and far more costly than the additional pass itself, which lead to #28534.

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.

None yet

5 participants