Skip to content

Fix GLSL type in LightCompoundData node#2552

Merged
jstone-lucasfilm merged 1 commit intoAcademySoftwareFoundation:mainfrom
tdavidovicNV:fix/tdavidovic/fix_float3_type_in_glsl
Sep 11, 2025
Merged

Fix GLSL type in LightCompoundData node#2552
jstone-lucasfilm merged 1 commit intoAcademySoftwareFoundation:mainfrom
tdavidovicNV:fix/tdavidovic/fix_float3_type_in_glsl

Conversation

@tdavidovicNV
Copy link
Copy Markdown
Contributor

In prep for the Slang merge, I was looking how close are the remaining Nodes between GenMsl and GenGlsl. And I found that the GenGlsl one was using float3 type instead of the vec3. I don't believe there is any alias for that, and even if there was, I think using the native type is the better idea. Also, it seems like there is no test coverage for this node, as I was running it with full tests (modified the _options to just point to resources/Materials) and this didn't come up.

Copy link
Copy Markdown
Contributor

@ld-kerley ld-kerley left a comment

Choose a reason for hiding this comment

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

This looks like it might have been a copy/paste bug I introduced - thanks for catching it.

You do raise a great question though - I wonder what the right way to get test coverage added for this code path is. @jstone-lucasfilm - do you have any insights there?

Copy link
Copy Markdown
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

Great catch, thanks @tdavidovicNV!

@ld-kerley Based on Tomas's description above, it sounds as if we don't yet have an example in our test suite that leverages this specific code path. Assuming we can express one through a new or extended MTLX file in our test suite, that sounds like the best path forward.

@jstone-lucasfilm jstone-lucasfilm changed the title Fixed a type where Glsl was using float3 instead of vec3 Fix GLSL type in LightCompoundData node Sep 11, 2025
@jstone-lucasfilm jstone-lucasfilm merged commit 7864cf3 into AcademySoftwareFoundation:main Sep 11, 2025
32 checks passed
@ld-kerley
Copy link
Copy Markdown
Contributor

Its always been a little unclear to me how the lights in MaterialX are intended to be used - so I was more asking if you have any concrete examples we can add - or have any insight as to how the lights are intended to be used, that would help me be able to construct some unit tests.

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