Implemented Part I of addExplicitTypeConversions()#2929
Implemented Part I of addExplicitTypeConversions()#2929christopher-jacobs wants to merge 7 commits into
Conversation
…begin implementing test conditions
Signed-off-by: jacobschristopher <chejac2000@gmail.com>
kwokcb
left a comment
kwokcb
left a comment
There was a problem hiding this comment.
This looks like a good start on this.
However, there are additional complications and scope....
- You can have "interface" inputs (input nodes) connected to internal node inputs.
- You have have internal node outputs connected to interface "outputs" (output nodes).
The "get" / "set" connected nodes will probably not work in these cases as they are not stricty node-node connections and also they will traverse to the parent.
- You need to consider not just
NodeGraphsbut "graphs" in general which is the parent classGraphElement. This will allow include ofDocumentswhich inherit fromGraphElement.
I will also add this to the issue, as this should be clarified there more succinctly. Apologies for the general language used.
|
Thank you for that feedback! I'll reiterate on this and make sure that I take these into consideration! |
|
As mentioned in the issue, I think it might be better to use some form of the validation logic to find the problem connections and then fix them afterwards. |
| /// Modify the nodes that can be converted explicitly within the associated NodeGraph. | ||
| /// @param addedNodes List of added nodes that were successfully integrated (w.r.t. the connection). | ||
| /// @param invalidConnections List of connections that could not be successfully converted. | ||
| void addExplicitTypeConversions(vector<NodePtr>& addedNodes, vector<NodePtr>& invalidConnections); |
There was a problem hiding this comment.
@kwokcb I'm curious stylistically if you think its important that this method exist on the NodeGraph class itself, or if instead we should consider it a true utility - and make it a stand-alone function.
In the back of my head I worry we start expanding the API surface of these objects, things may get harder in the future to manage API/ABI compatibility.
Also in any future world where we have the ability to feed ShaderGen with some other data model, if these "utility" functions existed stand-alone - we might be able to serve the same logic through the abstract interface, and say allow USD materials to also have conversion nodes inserted on demand.
There was a problem hiding this comment.
I don't see any reason that it needs to be on the core classes (*). It does need an input object (GraphElement) but that could be wrapped up into some opaque wrapper.
I'd prefer if these we're not global methods but instead add a "operation stack" base class which takes one or more "operations" (wrappers) to perform. You could create a MTLX version here and USD could create their own.
Is this kind of what your thinking of? ( Std disclaimer is that I haven't followed any of the ShaderGen refactor stuff of late.)
(*) @christopher-jacobs : This is not meant to overwhelm you with longer term complications. I think for this task it can just be on the GraphElement class.
There was a problem hiding this comment.
No worries, if anything I like observing this conversation and learning more about the different ways this could be implemented. I'll go ahead and implement the changes within GraphElement as we discussed earlier. I'm very interested though in this conversation and to explore and learn more! Thank you both!
4 participants
In regards to #2919 I implemented addExplicitTypeConversions() and added two tests to show a valid conversion and an invalid conversion. The valid test connects a vector3 output to a color3 output, and the invalid test attempts to connect a vector3 output to a float output. This currently loops through the entire NodeGraph to see if any conversions are possible, but I believe that this could be more easily implemented to be integrated with validate() (possibly taking the outputted message to find the questionable node and then check this connection directly).
This is my first time contributing to an open source project so let me know what can be improved and also any feedback. Looking at Parts II and III this is a very interesting and cool idea that I would love to continue if possible! Thank you!