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

Add API to allow input reordering on node graphs #2116

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

kwokcb
Copy link
Contributor

@kwokcb kwokcb commented Nov 15, 2024

Workflow Motivation

In order for users to specify interface ordering for node graphs they need to re-order the storage order of child inputs. Additionally, when creating new definitions (nodedef) they may which to specify the ordering of child inputs as part of definition publishing.

Proposal

Provide a simple atomic API to allow for inputs on node graphs to be re-ordered without affecting non-input child ordering.
The proposed API name is: NodeGraph::setInputOrdering() where a list all input names specifies the desired ordering.

@meshula
Copy link

meshula commented Nov 16, 2024

Would this need some work on the usd side to capture the ordering somewhere? Perhaps UsdUI or something?

@kwokcb
Copy link
Contributor Author

kwokcb commented Nov 17, 2024

Hi @meshula,
This doesn't add any additional metadata to specify the UI ordering. It's just a way to re-order inputs on a nodegraph.

Does USD already have separate specifiers for UI ordering (e.g. is this this stacking order in UsdUITokenType ?
I'm guessing from your comment it does not, but If it does then it might be worthwhile to look at the equivalent support for MaterialX as a related but separate change.

@meshula
Copy link

meshula commented Nov 17, 2024

Stacking order relates to the screen depth of nodes (this node should appear over that node if they overlap).

Reading the mx code, I see that childOrder is not a user imposed reordering of the nodes, but rather encodes a construction order. Since hdMtlx iterates that list, there's no reordering hints necessary at that end as order is implicit in that loop.

✅ #does-it-usd

@kwokcb
Copy link
Contributor Author

kwokcb commented Nov 25, 2024

Thanks for the explanation about stacking order.

To confirm your summary of childOrder:

  • Yes this is construction order but can serve double duty as "interface order" for inputs (not nodes).
  • This user ordering is optional for a compound graphs but from the various DCCs I've seen it's pretty well what folks are using for the UI. I've seen that Maya additionally allows for input re-ordering to modify user ordering.
  • When it comes to creating a new definitions from such a graph the input ordering is taken as a definition's input ordering so becomes "fixed" at that time -- i.e. if you change the ordering the definition is actually a different one.

Thus this utility to change construction ordering is being proposed to help support both the above workflows for user ordering.


That all said there is nothing required on the usd side. If at some point in the future definitions or graphs have additional meta-data to specify input ordering than this can be brought up.

@jstone-lucasfilm
Copy link
Member

@kwokcb Rather than making this a core method of the NodeGraph class in MaterialXCore, perhaps this would be better as a MaterialX Python API example, showing developers how they can build upon existing API methods such as Element::setChildIndex?

@ld-kerley
Copy link
Contributor

@jstone-lucasfilm - Do you believe that this operation would only be used by people authoring python code? It seems like there might be use cases outside of python, and providing it as part of the C++ API would allow everyone to benefit from a common implementation.

Maybe there are other reasons other than using it as a python API example that you think it shouldn't be part of the C++ API?

@jstone-lucasfilm
Copy link
Member

@ld-kerley My initial sense is that this new method is a very thin wrapper around the existing setChildIndex and getChildIndex methods, with a good deal of the new logic being dedicated to validating user inputs.

That makes me wonder whether it's better as a "best practices" example for new MaterialX developers, provided either in C++ or Python, rather than a core method that we provide along with setChildIndex and getChildIndex.

It's a very subjective sense on my part, though, and I'm open to a discussion of the benefits of this new API method in addition to our existing ones.

@ld-kerley
Copy link
Contributor

I guess I think things like input validation are exactly the sort of things it's good to share as part of the library, instead of leaving it to the users to implement themselves.

If the hesitation is to increase the API surface of the Nodegraph object itself - perhaps we could start creating free function that perform these helper tasks, but still include them as part of the library?

@jstone-lucasfilm
Copy link
Member

Yes, I was thinking that @kwokcb's new setInputOrdering represents a useful pattern for MaterialX developers, but that each use case is likely to be slightly different, e.g. ordering the inputs of an interface, ordering the nodes of a graph, ordering the top-level elements of a document, and so forth.

That might point to the idea of making this a "best practices" example somewhere in the MaterialX codebase, allowing developers to derive their own custom logic from this pattern, rather than making this a permanent feature of the NodeGraph API.

I'm definitely open to ideas on this, and we don't yet have an obvious home for best practices C++ examples in MaterialX, though I think having one would potentially be a real benefit to users.

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.

4 participants