Skip to content

Fix enumeration interfacename bug#2423

Merged
ld-kerley merged 2 commits into
AcademySoftwareFoundation:mainfrom
ld-kerley:fix-enumeration-interfacename-bug
Jun 3, 2025
Merged

Fix enumeration interfacename bug#2423
ld-kerley merged 2 commits into
AcademySoftwareFoundation:mainfrom
ld-kerley:fix-enumeration-interfacename-bug

Conversation

@ld-kerley

Copy link
Copy Markdown
Contributor

Related to #2422.

When creating the Input, during construction of the graph for code generation, currently if an enumeration is exposed via interfacename, then the value member ends up getting set to nullptr, which means later the code generation cannot determine the correct type.

This PR guards the setting of the value, to avoid setting this to nullptr.

@niklasharrysson niklasharrysson left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a good fix! Thanks @ld-kerley

@ld-kerley ld-kerley merged commit 153ee11 into AcademySoftwareFoundation:main Jun 3, 2025
32 checks passed
std::pair<TypeDesc, ValuePtr> enumResult;
const string& enumNames = nodeDefInput->getAttribute(ValueElement::ENUM_ATTRIBUTE);
const TypeDesc type = context.getTypeDesc(nodeDefInput->getType());
if (context.getShaderGenerator().getSyntax().remapEnumeration(valueString, type, enumNames, enumResult))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The "bug" appears to be in GLSL and MSL remapEnumeration method. It is returning true when there is no value mapped. MDL returns the correct value of false in this case.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Chatted offline with Bernard - we still think the original fix is valid - but I'm gonna put up a PR to fix the return value from remapEnumeration as well.

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