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

Behaviour when loading file is different to behaviour when re-running file #41

Open
hybridherbst opened this issue Jan 9, 2025 · 9 comments
Labels
bug Something isn't working

Comments

@hybridherbst
Copy link
Contributor

hybridherbst commented Jan 9, 2025

This file tests all math nodes for float types:
20250109-MathTests.zip

! This issue requires the fixes from our fork at the moment – registering missing nodes etc.

However, when initially loading it, it looks like this:
image

When pressing Play again, it looks like this:
image

I'm unsure what's wrong here. I believe the data is correct but maybe there's something else going on.

@boguscoder
Copy link

@hybridherbst quick question, unrelated to your observation but related to your file:
are inputs to ADBE/output_console_node spec - compliant? Id imagine it need to be
value: ["message"]
instead of
value : "message"
They fail in my parser hence the question

@boguscoder
Copy link

boguscoder commented Jan 9, 2025

Actually I just realized that no string value types are spec-compliant, but are often needed to test anything..

@hybridherbst
Copy link
Contributor Author

Yeah, good question, and I believe it depends on the spec for ADBE/output_console_node, which is basically an extension – we're just using that because this viewer here supports it. Where strings are used in the rest of the spec is as configurations, and configuration strings are never arrays, so I'm honestly not sure.

Unfortunately I don't know how to get Aero (where I believe the extension is coming from) to actually export a KHR_interactivity file...

@lexaknyazev for string types used in extensions, like the ADBE/output_console_node, do you have a recommendation? Should the value be stored as value: [ "some message"] or value: "some message"?

@boguscoder
Copy link

Its one of those things I asked to clarify in spec, I think values are never not-arrayed whether custom types or not (your test file also have few booleans that need to be put into array, I think)

Speaking of custom types, spec only mentions them for variables not for inputs, so its unclear if its even allowed

@mattmacf98 mattmacf98 added the bug Something isn't working label Jan 13, 2025
@mattmacf98
Copy link
Contributor

@hybridherbst can you link the fork so I can pull and test?

@hybridherbst
Copy link
Contributor Author

@mattmacf98 opened a PR so you can see them in one place:

@mattmacf98
Copy link
Contributor

mattmacf98 commented Jan 14, 2025

In the console I see

ADBE/outputConsole: Failed: math/lt. Expected: True. Actual: 
OutputConsole.ts:21 ADBE/outputConsole: true
OutputConsole.ts:21 ADBE/outputConsole: Failed: math/le. Expected: True. Actual: 
OutputConsole.ts:21 ADBE/outputConsole: true
OutputConsole.ts:21 ADBE/outputConsole: Failed: math/ge. Expected: True. Actual: 
OutputConsole.ts:21 ADBE/outputConsole: true
OutputConsole.ts:21 ADBE/outputConsole: Failed: math/isnan. Expected: True. Actual: 
OutputConsole.ts:21 ADBE/outputConsole: true
OutputConsole.ts:21 ADBE/outputConsole: Failed: math/isinf. Expected: True. Actual: 
OutputConsole.ts:21 ADBE/outputConsole: true

Which makes me think there is something weird going on for the value being provided by the Eq node

Also, when I log out console.log(a,b) in the equality node I get false true which is very strange since a is hardcoded to true in these tests, my guess is that on initial load, the true value in the inline math/eq is not getting evaluated properly

Now I have added some logging when I parse the type and see

Evaluating Bool: val[0]=undefined to false, val=true, so it looks like for inlined boolean values, I am not encapsulating the value in an array

@mattmacf98
Copy link
Contributor

mattmacf98 commented Jan 14, 2025

Screen.Recording.2025-01-14.at.6.34.52.PM.mov

Ok there were two issues here

  1. the AuthorToBehave's castParameter did not handle boolean values that are read from the glTF file (which are encased in the array)
    Updating castParameter to look like
const castParameter = (value: any, signature: string) => {
    switch (signature) {
        case "bool":
            return  typeof value === "string" ? [value === "true"] : value
        case "int":
        case "float":
            return [Number(value)];
        case "float2":
        case "float3":
        case "float4":
        case "float4x4":
            return typeof value === "string" ? stringToListOfNumbers(value) : value
        case "AMZN_interactivity_string":
            return String(value)
        default:
            return String(value)
    }
}

fixes that issue. Weirdly Number([20]) will actually unwrap the array for us and give us 20 which is why int and float parsin worked fine.

  1. the file you uploaded did not wrap the boolean values in arrays. e.x. they looked like
{"type":"math/eq","index":250,"values":[{"id":"a","type":0,"value":false},{"id":"b","node":249,"socket":"value"}]}

I have provided an example with properly wrapped boolean values.

This one only fails on the tests with inlined nans and Infinities on since JSON does not allow storing NaN and Infinity.

I will make a code change to parse the arrayish booleans properly tomorrow.

I think we should update these tests not to use inlined Infinity an Na as these are not allowed in JSON's spec

fixed_math_test.glb.zip

@mattmacf98
Copy link
Contributor

Screen.Recording.2025-01-14.at.6.56.27.PM.mov

One alternative Idea to hardcoding NaN and Infinity into the JSON is to just make them using math/div. This works great with the small change to parse arrayish bools and the test asset as checks for all cases!

math-fixed-final.glb.zip

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants