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

KHR_nodes_disable extension #1760

Closed

Conversation

UX3D-nopper
Copy link
Contributor

No description provided.

In general, all nodes with its child nodes are traversed and the meshes are rendered every frame. However, there are use cases, where specific meshes should be visible and some should be hidden. During runtime, these nodes are shown or others are hidden. A sample use case are products having specific attachments.
Goal is to have all meshes in the glTF and to show/hide specific meshes depending on the product situation.

If the visible property is `false`, the contents of the node are not visible or used. Main use case is not to render the mesh. By default, the change affects only the specified node. However, if the `recursive` property is set to `true`, all child nodes are affected as well.
Copy link

Choose a reason for hiding this comment

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

I would suggest to use a direct hiearachy approach and mandate that if a node (parent) is disabled then all children are disabled.
Ie, render traversal stops at the first Node that is disabled.
If this is not the intended behavior I suggest to add a disable property to the Mesh.

Copy link

Choose a reason for hiding this comment

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

USD has a similar solution for pruning of visible primitives (geometry)
A node will either have the value inherited or invisible.

Where invisible is always invisible and inherited will compute the visibility value for the parent.
In effect this will turn off all children when a node is hidden (by calling MakeInvisible)
The value is also animatable.

https://graphics.pixar.com/usd/docs/api/class_usd_geom_imageable.html#aa0dfa061a65be443436288f7406ba3db

Copy link
Contributor Author

@UX3D-nopper UX3D-nopper Feb 27, 2020

Choose a reason for hiding this comment

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

Think we should just make it the same behaviour.

@rsahlin
Copy link

rsahlin commented Feb 11, 2020

Hi @UX3D-nopper and thanks for creating the PR

I have a feeling that this is more of a runtime property than transmission property.
As I read the usecase in the proposal - the solution I see is a method in the viewer to disable/enable rendering of mesh(es). Not necesarily an addition to the glTF scenegraph.

I see the data needed to toggle nodes as configuration data and not part of glTF.

The glTF usecase I see for toggling visibility would be to add support for an animation that affects node visibility (disable/enable flag)
That way the model could be queried for animation (state) and use that information to know what parts are visible or removed - by means of configuration logic.

@UX3D-nopper UX3D-nopper added the needs discussion Issue or PR requires working group discussion to resolve. label Feb 27, 2020
@rsahlin rsahlin mentioned this pull request Apr 25, 2020
@edzis
Copy link

edzis commented May 4, 2020

To share context with people outside the 3D Commerce working group, here is what I said in the email thread:


I fully support the need to solve visibility that Norbert is tackling. I also agree that nodes seem to be the best target for that. I also believe this can be useful beyond Configurability as a means to hand over assets between artists without needing to show everything.

But I believe that to make this relevant for Configurability this extension needs to have a dynamic nature beyond just specifying the visibility value at authoring time.

I fully support Paul's comment on enabled/disabled. In our configurators using Babylonjs we use setEnabled() instead if isVisible because of the same implementation details that Paul mentioned. Also, that naming would match better with other things besides meshes - cameras, groups, etc.

On top of that I want to make sure this is easy to use in scenarios of multiple Properties affect visibility for sections of the node graph at different levels of depth.

Considering both of these threads I recommend calling this extension KHR_disable_node which is recursive by nature. That would turn off any nested children as soon as a node is disabled.

I believe that makes the mental model simple, intuitively communicating that a child node with disabled not active would still not be visible as soon as one of it's ancestors is disabled.

Also, instead of just specifying the disabled value statically:

"KHR_disable_nodes": {
  "disabled": true
}

... I recommend to add ability to connect that to the same system of tags that would be used for material variants.

We might disable something based on any of the tags being set:

"KHR_disable_node": {
  "whenTag": ["faceguard:none", "faceguard:padLeft", "faceguard:mask"]
}

... or, if we don't want to list all the negative tags, keep something disabled unless a specific tag is set:

"KHR_disable_node": {
  "unlessTag": ["faceguard:padRight"]
}

@mixtur
Copy link

mixtur commented Apr 22, 2021

I was looking for something like that. #80 seems to be somewhat related

We have a concept of "views" in our app. A view is a camera plus a set of nodes to render.

So my suggestion would be to allow adding some kind of "visibility tag" to nodes and also cameras. This seems similar to what @edzis suggested. Although I am not entirely sure how it must affect subtrees. I think it is a point of controversy, different people may have very different ideas about it. Maybe the behavior there should be either more flexible or more restrictive.

More flexible would be to add an option that defines how the subtree is affected.
More restrictive for example would be only allowing the extension on leaf nodes. Which is almost the same as attaching the extension to meshes instead of nodes.

I personally don't like the "flexible" option, I have seen this in another format and it was very hard to figure what is visible and what is not there. But businesses may require crazy things)

UPD: only allowing it for leaf nodes won't work because a node may have both mesh and children.

| Name | Type | Default | Description
|-------------|-----------|---------|-----------------------------
| `visible` | `boolean` | `true` | Node is visible or not.
| `recursive` | `boolean` | `false` | Valid for children as well.
Copy link
Contributor

Choose a reason for hiding this comment

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

Many engines do not support non-recursive behavior, so it can be a lot of work to implement. I personally would suggest we do not support non-recursive behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason why it is hard to support is that if the engine does not support recursive visibility, you need to add new nodes for the main hierarchy with meshes as child nodes. But this screws up trying to do animations, etc, because now there is additional nodes.

@aaronfranke
Copy link
Contributor

This PR is superseded by KHR_node_visibility, so this PR can be closed.

The key difference is that KHR_nodes_disable allows recursive and non-recursive visibility, while KHR_node_visibility only has a single property for visibility and is always recursive. As discussed above and pointed out by @bhouston, the non-recursive behavior is not desired, so we only want the recursive behavior, which is what KHR_node_visibility does.

@emackey emackey closed this Aug 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
extension needs discussion Issue or PR requires working group discussion to resolve.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants