-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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_node_visibility Draft Proposal #2410
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm lacking some context from the KHR_animation_pointer
/KHR_interactivity
, but this looks pretty straightforward.
Maybe hidden
should be required
, but when default
ing to false
, this may not be strictly necessary.
Having a default value is exactly why it's not required. So to make node's visibility controllable, an empty extension object is all that's needed. |
This sort of "Inverted Boolean" is a design pitfall that I usually lobby against, for the sake of non-programmer users who are sometimes confused by double-negatives. Please take a moment and really think through the implications of changing this to parameter Currently this draft's Apologies if this is just a pet peeve of mine. I've been opposed to this kind of Boolean use since the late 90's when some software I work on sported a checkbox on the UI that looked like this:
There was a similarly named |
@emackey That was also the first thing that I thought. And I fully agree that this is an antipattern: Nobody wants things like |
Consider the following:
Out of all possible combinations of the property direction (positive/negative) and the default value (true/false), only two options remain:
An optional boolean JSON property has three states:
If let ext1 = {visible: false}
let ext2 = {visible: true}
let ext3 = {};
// Naive check
if (ext1.visible) {} // ok
if (ext2.visible) {} // ok
if (ext3.visible) {} // wrong
// Inverted naive check
if (!ext1.visible) {} // ok
if (!ext2.visible) {} // ok
if (!ext3.visible) {} // wrong
// Accurate check
if (ext1.visible !== false) {} // ok
if (ext2.visible !== false) {} // ok
if (ext3.visible !== false) {} // ok
//
let ext4 = {hidden: false}
let ext5 = {hidden: true}
let ext6 = {};
// Naive check
if (ext4.hidden) {} // ok
if (ext5.hidden) {} // ok
if (ext6.hidden) {} // ok
// Inverted naive check
if (!ext4.hidden) {} // ok
if (!ext5.hidden) {} // ok
if (!ext6.hidden) {} // ok The same logic applies to other languages that may not even have a concept of an undefined boolean. DCC tools do not have to present the flag literally so the double-negative UI could be easily avoided in user-facing tools. |
Yes. This is the one I'm advocating for here:
Yes, JavaScript and JSON have some interesting wrinkles when distinguishing between
In your code samples, sure. But I'm talking simple human understanding here. I'm talking about people trying to learn foreign design details of unfamiliar standards. There's no wiggle room here to say that the default behavior of displaying ordinary glTF nodes is somehow a negative action, and that hiding them from view is positive. It's clearly inverted. The cleaner design is to use |
That's an interesting euphemism for "irrecoverably broken clusterf..." 🙂 I already suspected that the reasoning was in some form of "backward compatibility". And this indeed is the reason here - namely, via the statement
which causes a problem (only) in combination with the case of
Now... one could argue about that. One could call this "an implementation error". And something like this is less likely to happen for experienced developers who know the ... wrinkles... of JS. However, there's certainly that trade-off between the more intuitive approach of If we chose to use the |
The important thing from a functionality perspective is that it must be possible to hide a subtree of the nodes with a single value change, since we don’t provide constructs for iterating an entire tree, it would inefficient to do so, and it could lose important state at lower levels (such as where a diorama is to be made visible and the animation within the diorama involves flipping the visibility state of some diorama elements – we must be able to toggle on the visibility of the diorama as a whole while allowing the animation track to have unfettered control of visibility within the diorama).
So, if you have “visible” with default “true”, then the logic for whether any node should display must be “display node if and only if node and all parents have visible=true (including by default)”.
This means that a node X may be hidden despite having “visible=true”. To me, it feels slightly more intuitive to reason that “node X is visible even though hidden=false, because its parent is hidden” than it is to reason that “node X is invisible even though visible=true, because its parent is invisible”, but I don’t have a strong opinion here. Mostly I want to point out that either way people will have to learn the design detail of how the effect of the property propagates in the hierarchy, which is the most important thing.
Dwight
From: Ed Mackey ***@***.***>
Date: Thursday, June 13, 2024 at 8:49 AM
To: KhronosGroup/glTF ***@***.***>
Cc: Dwight Rodgers ***@***.***>, Review requested ***@***.***>
Subject: Re: [KhronosGroup/glTF] KHR_node_visibility Draft Proposal (PR #2410)
EXTERNAL: Use caution when clicking on links or opening attachments.
Yes. This is the one I'm advocating for here:
* Property name visible with default value true
Yes, JavaScript and JSON have some interesting wrinkles when distinguishing between undefined, null, false, 0, and the empty string. My asking for a default value of true will feed into this problem, but it's well-known by JS developers (and I have experience with how these issues were routinely handled from my time on the early Cesium team and other WebGL projects). We do already have a case where undefined defaults to infinity (attenuation distance if I recall), and another case where undefined defaults to 0.5 (which happens in alphaMask I believe). So, this should not place any undue burden on developers. It will be immediately apparent if some client application defaults visibility to false by accident.
it may be an open question whether "hiding a node (tree)" is a positive or negative action.
In your code samples, sure. But I'm talking simple human understanding here. I'm talking about people trying to learn foreign design details of unfamiliar standards. There's no wiggle room here to say that the default behavior of displaying ordinary glTF nodes is somehow a negative action, and that hiding them from view is positive. It's clearly inverted. The cleaner design is to use true as the default state here for displaying visible nodes.
—
Reply to this email directly, view it on GitHub<#2410 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AM7FVTK5RDH7AUGJIYYYWZ3ZHG5PHAVCNFSM6AAAAABJGJVKIWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRWGA3DINRSGI>.
You are receiving this because your review was requested.Message ID: ***@***.***>
|
All good points. And @lexaknyazev does show the correct code for checking this: // Accurate check
if (ext1.visible !== false) {} // ok
if (ext2.visible !== false) {} // ok
if (ext3.visible !== false) {} // ok Yes, implementers will still need to pay attention to the details and the hierarchy. But we can easily supply test asset(s) that will make it very plain to see if an entire node has gone missing or inappropriately shown. The folks I'm concerned about are the adopters: The users, the artists, the designers. They use this spec too, and they can't be kept in line with a simple unit test like programmers can be. And this is an instance of a known design antipattern for them that seems easy to avoid. I don't want to stand in the way of progress here, so if the consensus is to dismiss my objection, I can accept that. But so far in this thread I have only seen, pardon my expression, programmer reasoning here, coder's logic, which is not always along the same lines as how the artists and creators like to think. |
Agreed within the Interactivity DTSG to use the "positive" property name provided that test assets for this extension will include multi-node objects relying on the implicit (default) property value. |
3a46d03
to
52d04b5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a strong opinion for either hidden
or visible
. The trade off is that the latter is more "intuitive", and the former avoids that possible if (!node.visible) shouldDefaultToTrue();
bug. (Maybe there could be an 'Implementation Note' about that - but that may not be too important). So the new approach is fine for me.
A quick attempt to create a test asset: KHR_node_visibility test 0001 - 2024-06-26.zip
The behavior is "emulated" in BabylonJS here: There's probably room for improvement, but maybe someone wants to give it a spin... |
For content creation tools exporting glTF files, is it recommended that files using |
That should depend on the intended usage. Couple examples:
|
I have implemented this in Godot Engine here: godotengine/godot#93722. Both import and export are supported, so Godot Engine can be used for both authoring and consuming glTF assets with Test file: cube_visibility_example.zip |
"type": "boolean", | ||
"description": "Specifies whether the node is visible.", | ||
"default": true, | ||
"gltf_detailedDescription": "Specifies whether the node is visible. A value of false means that the node and all its children are hidden." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick
"gltf_detailedDescription": "Specifies whether the node is visible. A value of false means that the node and all its children are hidden." | |
"gltf_detailedDescription": "Specifies whether the node is visible. A value of false means that the node and all its descendants are hidden." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also considered to bring that up, but... it transitively applies to all relevant nodes, so this does not change the meaning on a technical level. (One could argue which of them is easier to understand, conceptually, but I don't have a strong opinion there...)
|-------------|-----------|----------------------------------------|--------------------| | ||
| **visible** | `boolean` | Specifies whether the node is visible. | No, default: `true`| | ||
|
||
In other words, a node is visible if and only if its own `visible` property is `true` and all its parents are visible. This allows a single change of a `visible` property at a high level of the hierarchy to hide or show complex (multi-node) objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, a node is visible if and only if its own `visible` property is `true` and all its parents are visible. This allows a single change of a `visible` property at a high level of the hierarchy to hide or show complex (multi-node) objects. | |
In other words, a node is visible if and only if its own `visible` property is `true` and all its ancestors are visible. This allows a single change of a `visible` property at a high level of the hierarchy to hide or show complex (multi-node) objects. |
|
||
The `KHR_node_visibility` extension object is added to the objects within the `nodes` array. The extension object contains a single boolean `visible` property. This value is mutable through JSON pointers as defined in the glTF 2.0 Asset Object Model and controls visibility of the node that contains it and all its children nodes recursively. A value of `false` causes all nodes below in the hierarchy to be omitted from display, even any nodes below that have a value of `true`. | ||
|
||
| | Type | Description | Required | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The titles here are center-aligned, but they are left-aligned in most files in this repo.
|
||
| | Type | Description | Required | | ||
|-------------|-----------|----------------------------------------|--------------------| | ||
| **visible** | `boolean` | Specifies whether the node is visible. | No, default: `true`| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| **visible** | `boolean` | Specifies whether the node is visible. | No, default: `true`| | |
| **visible** | `boolean` | Specifies whether the node is visible. | No, default: `true` | |
...and modify the other lines to keep this aligned.
As discussed in the Interactivity DTSG, there should be a way to control node visibility through
KHR_animation_pointer
and/orKHR_interactivity
workflows.