three.js

three.js copied to clipboard

Fix #18937

This PR changes layers to apply recursively -- so if a parent object does not pass the layer test then raycasting and rendering traversal will stop. This will let users treat groups of objects as a whole and is more consistent with the way object visibility works.

Ping @finnbear to see if this addresses your usecase, too.

As outlined earlier (see https://github.com/mrdoob/three.js/issues/18937#issuecomment-602026028), I do not vote to make this change and leave the code as it is for greater flexibility.

@gkjohnson yes this would solve my issue. My only comment is that, if layers are recursive, the current default layer mask is flawed. In my opinion, the default should be all layers, just as the default "visibility" is true.

Otherwise, to match a camera on layer 2 only, the user would have to traverse the tree to set the layer of each parent to 2 ([**each** parent].layers.set(2)), for example, instead of setting one parent to 2 only ([**any** parent].layers.set(2)). This is better than the alternative of setting each of the many more children to not be visible, though.

Another alternative is to default layers to 0b11111111111111110000000000000000 (first 16 default on, last 16 default off) to have even greater flexibility at the expense of being a tiny bit arbitrary.

I do not vote to make this change and leave the code as it is for greater flexibility.

Currently, Layers can only be manipulated additively. In other words, you can add objects to a layer but there is no way to bulk-remove objects from a layer. This PR would add a way to manipulate layers subtractively, as a consequence of the object hierarchy. With the change to the default layer I suggested, the additive method would take the same number of api calls. To add an object to layer 2, you would set that object's individual layer to 2, leaving the parent as-is (default all layers). To restrict an object to layer 2, you could disable layer 1. While the API call for the additive method would be slightly different, it is still one call per child object.

Supporting both the additive and subtractive method seems more flexible to me.

Just thought of one more thing. Three.js allows the user to specify Object3D.DefaultMatrixAutoUpdate;. In the same way, it could allow the user to specify Layers.DefaultMask. If this were the case, flexibility would be much higher (especially with this PR merged).

For a completely additive approach: (the only two workflows that are possible now)

// Workflow 1
Layers.DefaultMask = 1;
var object1 = new THREE.Object3D();
object1.layers.enable(2); // Selectively add object1 and its child, but not object2
object1.add(child);
var object2 = new THREE.Object3D();

// Workflow 2 (This is the only impossible workflow after the PR, and it happens to be least common)
Layers.DefaultMask = 1;
var mesh1 = new THREE.Mesh();
var child2 = new THREE.Mesh();
object1.add(child2);
child2.layers.enable(2); // Selectively add child2 but hide its parent, mesh1

For a completely subtractive approach: (2nd camera on layer 2 only)

// Workflow 3
Layers.DefaultMask = 0xffffffff;
var object1 = new THREE.Object3D();
var child = new THREE.Mesh();
object1.add(child);
object1.layers.disable(2); // Selectively remove object1 and its child but keep object2

// Workflow 4
Layers.DefaultMask = 0xffffffff;
var mesh1 = new THREE.Mesh();
var child2 = new THREE.Mesh();
object1.add(child2);
child2.layers.disable(2); // Selectively remove child2 but keep its parent, mesh1

For a mixed approach: (camera 1 has mask 5 and camera 2 has mask 10)

// Workflow 5
Layers.DefaultMask = 0xffffff00;
var object1 = new THREE.Object3D();
var child1 = new THREE.Mesh();
object1.add(child1);
child1.layers.disable(10); // Selectively remove child 1 for camera 2
var child2 = new THREE.Mesh();
object1.add(child2);
child2.layers.enable(5); // Selectively enable child2 for camera 1

// Workflow 6
Layers.DefaultMask = 0xffffff00;
var object1 = new THREE.Object3D();
var child1 = new THREE.Mesh();
object1.add(child1);
child1.layers.disable(10); // Selectively remove child 1 from camera 2

class DefaultHiddenMesh extends THREE.Mesh {
  constructor(..args) {
    super(...args);
    this.layers.disableAll();
  }
}

var child2 = new DefaultHiddenMesh();
child2.layers.enable(5); // Selectively enable child2 for camera 1

The only missing functionality, as I see it, is if a Mesh (mesh1) is the child of another Mesh (mesh2), and you want to show mesh1 but hide mesh2. That would not be possible but I don't think its a real/common use case for layers. As I see it, Meshes would most commonly be the children of non-rendering THREE.Object3D's or THREE.Group's.

In summary, one uncommon workflow is rendered impossible but four more common workflows are made possible.

@Mugen87

As outlined earlier (see #18937 (comment)), I do not vote to make this change and leave the code as it is for greater flexibility.

I saw and I tried to inquire further (https://github.com/mrdoob/three.js/issues/18937#issuecomment-602252153). Can you explain how you feel it's more flexible and what is lost by supporting this feature? As it is there seem to be no use cases that use parent node layers so this change will make them more powerful and capable than before. This PR gives a purpose to setting the layers of a parent node which are otherwise useless right now. I also think the behavior being different from visible is a bit confusing.

I don't see this logic as application specific. Not every application is going to be managing every single object in the hierarchy. I see this as a tool for enabling encapsulated objects as extensions that can manage themselves and be treated as a whole.

It's conceivable that some parent mesh could be disabled via layers while children are still enabled. I'd be hesitant to make this change as it's a breaking change, at least unless a way to make the new behavior optional is included.

I think we could add layers.recursive = true; // false by default

@mrdoob

I think we could add layers.recursive = true; // false by default

Thanks that works for me! I am curious as to the reasoning behind wanting to keep the layers applying to leaves only by default when they otherwise have no influence on parent nodes. I just want to clarify so I can make more reasonable and agreeable contributions in the future. If backwards compatibility is a concern then defaulting parent nodes to layers 0xFF would address that, too. Either way if layers.recursive is a compromise that makes everyone happy then I support it. I can make the changes later this week.

Thanks that works for me! I am curious as to the reasoning behind wanting to keep the layers applying to leaves only by default when they otherwise have no influence on parent nodes.

As far as I understand, that's how Blender layers behave. I don't really have a preference but I would align to Blender, Unity, Unreal, etc

@mrdoob

I just made the changes! If object.layers.recursive is true then that parents layers will affect rendering and raycasting of children.

I've added a unit test for raycasting and tested rendering by modifying the webgl_loader_gltf example with these lines after loading the helmet :

gltf.scene.layers.disableAll();
gltf.scene.layers.recursive = true;

@mrdoob any further thoughts on this PR? If needed I can try to explain the logic more clearly. Would it help if I separated the separated the conditions instead of putting all the logic in a single boolean statement?

It also occured to me that that condition could be simplified to object.layers.recursive === false || layerTest if that's better.

@mrdoob

I think we could add layers.recursive = true; // false by default

Thanks that works for me! I am curious as to the reasoning behind wanting to keep the layers applying to leaves only by default when they otherwise have no influence on parent nodes. I just want to clarify so I can make more reasonable and agreeable contributions in the future. If backwards compatibility is a concern then defaulting parent nodes to layers 0xFF would address that, too. Either way if layers.recursive is a compromise that makes everyone happy then I support it. I can make the changes later this week.

Hi, thanks for this great feature, when will this be implemented? I really need this. checking layers' setting one by one, or do it with extra logics / testings is quiet a bit boring. @mrdoob @gkjohnson @Mugen87

I did not make it clear during the discussion but having a separate recursive property that controls whether layers are evaluated recursively or not is something I do support. This should hopefully satisfy devs who rely on the current system and the ones who desire an easy to use recursive approach.