-
Notifications
You must be signed in to change notification settings - Fork 397
Graph Editor - UI support for enum #2395
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
base: main
Are you sure you want to change the base?
Graph Editor - UI support for enum #2395
Conversation
Thanks @inter-sakot4, and I'll link this to GitHub Issue #2366. |
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.
This looks very promising, @inter-sakot4, and I've added a note on the line that appears to be triggering build issues on some platforms.
|
||
// parse string values from the enum, enumvalues attribute directly and populate enumValues | ||
enumVec = mx::splitString(enumStr, enumDelimiter); | ||
int groupSize = mx::splitString(enumValueStr, enumDelimiter).size() / enumVec.size(); |
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.
Here's the line that likely needs an update for cross-platform compatibility, since the return type of vector::size
is size_t
.
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.
Thankyou for taking time to review @jstone-lucasfilm ! Fixed the type issue as suggested and added short comments for clarity.
bool Graph::renderEnumInput(mx::InputPtr& input, const mx::UIProperties& ui, T &evalue) | ||
{ | ||
|
||
// Eg. <input name="subsurface_color" type="color3" value="0.2,0.6,0.05" |
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.
Although this is a minor style issue, I notice that this method uses 2-space indentation rather than our standard 4-space indentation, and that would be valuable to fix before we include this change in the codebase.
std::vector<mx::ValuePtr> enumValues = ui.enumerationValues; | ||
std::vector<std::string> enumVec = ui.enumeration; | ||
|
||
//Handling edge cases where enumvalues attribute is empty |
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 notice some minor inconsistencies in these comments, and it would be valuable to use the same spacing and capitalization for all of them. Usually, we use a single space after the initial //
marker, and the first letter of the first sentence is capitalized.
@@ -145,6 +145,7 @@ unsigned int getUIProperties(InputPtr input, const string& target, UIProperties& | |||
propertyCount++; | |||
} | |||
|
|||
//Enumeration fields are not populated for non uniform inputs. Is this intentional? This affects the issue #2366 |
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.
This looks like a useful note, and I'll CC @lfl-eholthouser for her thoughts, though we should make sure not to include this when the change is ultimately merged to main.
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.
Worth noting that currently MaterialXWebViewer does show enum/enumvalues for non-uniform inputs.
It probably should not, given the spec, but I'm not sure which one is right.
I closed my related PR #2438 because yours looks much more complete, but my test file does not actually show a dropdown UI when I build from this PR @inter-sakot4. Here's the test file that doesn't bring up a dropdown: |
@inter-sakot4 Just following up on this PR, to see if you might have a chance to address the issue brought up by @hybridherbst above! |
Description
In this PR, UI support for enumerator types are added to GraphEditor.
Some assumptions are made to execute this task:
With the above assumptions, scope of this task is limited only to GraphEditor ( IMGUI )
Solution
renderEnumInput<input_type>(input, uiProperties, temp)
is a template function that does the rendering of enumerator input types.Following edge cases are handled:
How to test?
Slightly modified
standard_surface_jade.mtlx
can be used like below.Expected Result