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

[DRAFT] glTF interactivity and flow graph update (For snapshot) #15810

Draft
wants to merge 141 commits into
base: master
Choose a base branch
from

Conversation

RaananW
Copy link
Member

@RaananW RaananW commented Nov 13, 2024

I open this PR just to update the snapshot.
No reviews needed, as it will be closed right after.

@hybridherbst
Copy link

hybridherbst commented Nov 13, 2024

Thank you, I was able to do some tests. The built version seems to hide exceptions, so I tested locally as well.
Here are some test files:

  1. Setting KHR_node_visibility pointers doesn't work

  2. Setting visibility every few moments with a delay node

Footnotes

  1. My unsuccessful attempt at adding this (and the types) in objectModelMapping.ts:

         extensions: {
            KHR_node_visibility: {
                visible: {
                    get: (node: INode) => (node instanceof AbstractMesh ? node.isVisible : true),
                    set: (value: boolean, node: INode) => {
                        // eslint-disable-next-line no-console
                        console.log("setting visibility", value);
                        if (node instanceof AbstractMesh) {
                            node.isVisible = value;
                        }
                    },
                    getTarget: (node: INode) => node._babylonTransformNode,
                    getPropertyName: [() => "isVisible"],
                    type: "boolean",
                },
            },
        },
    

@hybridherbst
Copy link

Do you want me to open issues against your fork instead? If so, please enable Issues there :)

@RaananW
Copy link
Member Author

RaananW commented Nov 14, 2024

Do you want me to open issues against your fork instead? If so, please enable Issues there :)

Would actually be great to keep it outside of github. the forum would be the best place for this.
We use github a bit differently. Extensions tree was not yet (fully) implemented and is on my list for this week, so it will probably be done today or tomorrow.

I fixed it. value should be an array, even if it is primitive (or at least this was a discussion we previously had). Not written in the specs though, so - I made sure a non-array value is now parsed correctly. this is the reason it didn't read the value correctly. I also added the KHR_node_visibility to the nodes map, so it is now working (on start and the once a second).

The snapshot should update in a few minutes so you can test it.

If you have any further test data you think are not working correctly, let me know. Forum would be the best, but here is fine as well. As you wish TBH.

@hybridherbst
Copy link

It seems I'm not yet allowed to upload files on the forum, so here's some more failing test files. They work in the Graph Authoring Tool implementation. Upon clicking the blue button the traffic light should go red-yellow-green-yellow-red.

I believe these could have the same root issue.

@RaananW
Copy link
Member Author

RaananW commented Nov 18, 2024

It seems I'm not yet allowed to upload files on the forum, so here's some more failing test files. They work in the Graph Authoring Tool implementation. Upon clicking the blue button the traffic light should go red-yellow-green-yellow-red.

I believe these could have the same root issue.

Quick note on the files you provided, there is an error in the glTF that partly prevents us from parsing the types correctly (interpolation demo). The pointer's configuration is this:

"configuration":[{"id":"pointer","type":11,"value":"/materials/1/pbrMetallicRoughness/baseColorFactor"}], (expected float4)
and the value is this:

"values":[{"id":"value","type":7,"value":[0.9921569,0.7337824,0.0,1.0]}

value is a float4, but type:7 is float3. There are duplicated float3 as well, but we don't check that yet, so that's perfectly fine. So even if I make sure to parse vector4/vcolor4 correctly, this will fail. I will try finding a proper solution for this.

Anyhow, the set is already fixed (as I suspected it was an issue with vector-to-color), interpolation is being worked on.

@hybridherbst
Copy link

hybridherbst commented Nov 18, 2024

Thanks for fixing, good catch about float3/float4! – here's a file with the corrected type for color:
TrafficLight_Simple_SetColorPointer2.glb.zip

Not sure if the sandbox should already be updated – I can see the "red" color correctly but upon clicking the blue button the colors still go to some kind of dark purple.

I think the float3/float4 mixup might happen more often, as colors (typically float4 in all systems) can be either float3 or float4 in glTF, depending on usage, so I expect exporters to mess that up occassionally...

@RaananW
Copy link
Member Author

RaananW commented Nov 18, 2024

Thanks for fixing, good catch about float3/float4! – here's a file with the corrected type for color: TrafficLight_Simple_SetColorPointer2.glb.zip

Not sure if the sandbox should already be updated – I can see the "red" color correctly but upon clicking the blue button the colors still go to some kind of dark purple.

I think the float3/float4 mixup might happen more often, as colors (typically float4 in all systems) can be either float3 or float4 in glTF, depending on usage, so I expect exporters to mess that up occassionally...

The sandbox will be updated in 15-20 minutes (waiting for the build to finish). Interpolation should also be working now.

The new file doesn't reset the colors to gray, so it only shows red yellow and green. Is that the correct behavior?

@hybridherbst
Copy link

Sorry, it was indeed not the correct file.

✅ This should be correct now (and works in the updated sandbox – thanks!):

❌ The pointer interpolation file does not behave correctly yet –

  • File: TrafficLight_Simple_Interpolate.glb.zip

  • Expected Actual
    Screen.Recording.2024-11-18.at.17.26.40.mov
    Screen.Recording.2024-11-18.at.17.27.42.mov
  • Problems:

    • Interpolation flickers (whenever a pointer interpolation starts, the value seems to "snap" somewhere else for a frame)
    • my guess is that pointers never "stop" on their own after being done interpolating – thus the transition backwards (green-yellow-red) doesn't have any effect. I believe that independent of whether the transition is already done or not, newer pointer/interpolate calls should beat older ones as per the spec

@RaananW
Copy link
Member Author

RaananW commented Nov 18, 2024

  • Interpolation flickers (whenever a pointer interpolation starts, the value seems to "snap" somewhere else for a frame)
  • my guess is that pointers never "stop" on their own after being done interpolating – thus the transition backwards (green-yellow-red) doesn't have any effect. I believe that independent of whether the transition is already done or not, newer pointer/interpolate calls should beat older ones as per the spec

That is true, I am not tracking currently running interpolation. Not yet, at least.

@bjsplat
Copy link
Collaborator

bjsplat commented Nov 20, 2024

@RaananW
Copy link
Member Author

RaananW commented Nov 20, 2024

  • Interpolation flickers (whenever a pointer interpolation starts, the value seems to "snap" somewhere else for a frame)
  • my guess is that pointers never "stop" on their own after being done interpolating – thus the transition backwards (green-yellow-red) doesn't have any effect. I believe that independent of whether the transition is already done or not, newer pointer/interpolate calls should beat older ones as per the spec

Both were fixed in the last commit. The issue was not the interpolation running already. The issue was reusing already-existing blocks, which was prevented deliberately. I believe there was a discussion about that a long time ago (and this is why it was implemented this way originally), but I can't find it in the current state of the specs. So - blocks can now be reused and re-triggered.
The flickering was because of an issue we had in our bezier curve implementation, and that was fixed as well.

@hybridherbst
Copy link

hybridherbst commented Nov 20, 2024

Here's a test file that contains additional nodes not known by the engine. My understanding as per the spec discussions is that these should be ignored, as they could be defined in extensions that the engine simply doesn't support at the moment.

SimpleMath_SubGeneric_Logging.glb.zip

Looks like this – the goal here is to support logging in multiple engines that have different nodes for logging:

image

Currently, it causes an exception because the node is not known.

@RaananW
Copy link
Member Author

RaananW commented Nov 21, 2024

Here's a test file that contains additional nodes not known by the engine. My understanding as per the spec discussions is that these should be ignored, as they could be defined in extensions that the engine simply doesn't support at the moment.

Should be fine now.

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