Definition and NodeGraph Publishing Logic Updates #1303
Conversation
…e support for nodegraphs for existing interface methods.
…rface to the input before removing the interface.
|
@jstone-lucasfilm , I was trying to add / remove interfaces and publish nodedefs and found that the current code isn't robust and will result in validation errors. Also this fixes up all the attribute transfer to / from interfaces on nodedefs / functional graphs / compound graphs as far as I can see. There is no place which codifies the "rules" except for here AFAIK. |
resources/Materials/TestSuite/stdlib/definition/definition_from_nodegraph.mtlx
Show resolved
Hide resolved
|
|
|
/easycla |
…aterialX into nodedef_API_spec_updates
…' location unless explicitly specified on the command line where to put things.
…same logic fixes. Fix up optional args in Python interface.
|
@jstone-lucasfilm, I've added in the new interface |
|
@kwokcb From a high level, I think that this changelist may be combining too many independent ideas, making it challenging to bring all of them to the required level of polish in a single pull request. What would you think about breaking this out into separate pull requests, perhaps starting with an improved, streamlined version of |
Actually, you had requested that there be a workflow for definition creation :) So two logical places were a script and interactive in the node editor. I think this has helped to refine the requirements and signature. I don't think there is much additional in the node editor interface, except that it sets up a convention for naming of the nodegraph and nodedef i.e. ND_category_, and NG_... For the former point, making the arguments explicit and atomic allows for facilitating unique signature creation as mentioned in the interface comment above. |
|
That's a very fair point, @kwokcb, and I think I was hoping that these concrete use cases would help to clarify the motivation for the proposed functionality in MaterialXCore, though so far I'm not quite seeing the benefits. Let me take a closer look at these proposals, and I'll see if there are any specific refinements that might be worthwhile to suggest. |
| options.newNodeDefName = newNodeDefName; | ||
| options.newNodeGraphName = newGraphName; | ||
|
|
||
| mx::NodeDefPtr nodeDef = doc->createDefinition(options); |
There was a problem hiding this comment.
This seems like a great use case for the new createDefinition functionality, so let's focus on this one for now. In this situation, I would imagine it's clearer and more straightforward for a client to call the following:
mx::NodeDefPtr nodeDef = doc->createDefinition(graph);
nodeDef->setVersion("1.0");
nodeDef->setIsDefaultVersion(true);
This makes it clear that nearly all of the required data (e.g. name, category) is being derived from the NodeGraph itself, with only the versioning data for the NodeDef being stated explicitly by the client.
What are the benefits that you see in the DefinitionOptions class for this use case? I could see it being useful if a single DefinitionOptions were constructed once and then reused multiple times, but that doesn't seem like a common situation that clients would encounter in practice.
Very interested in your thoughts on this!
There was a problem hiding this comment.
This seems like a great use case for the new
createDefinitionfunctionality, so let's focus on this one for now. In this situation, I would imagine it's clearer and more straightforward for a client to call the following:mx::NodeDefPtr nodeDef = doc->createDefinition(graph); nodeDef->setVersion("1.0"); nodeDef->setIsDefaultVersion(true);This makes it clear that nearly all of the required data (e.g. name, category) is being derived from the NodeGraph itself, with only the versioning data for the NodeDef being stated explicitly by the client.
What are the benefits that you see in the DefinitionOptions class for this use case? I could see it being useful if a single DefinitionOptions were constructed once and then reused multiple times, but that doesn't seem like a common situation that clients would encounter in practice.
Very interested in your thoughts on this!
- For the first example, the main worry here is what if someone does this and then changes the version. This also involves the unique signature semantics being used. i.e. the proposal is the the identifier includes version. so you'd end up with something like
ND/NG_foo_<version>_....etc
IMO, the user should not need to know where the information resides (whether it's on the graph or definition), though I agree that the options should probably be nodedef specific and not include the graph so it would be more of:
createDefinition(graph, nodedef_options)
I wanted to change this anyways as there is possible reuse when dealing with source code implementations -- i.e. the nodedef options are still valid.
- An example for the second case is if you want to create variants on definition. e.g. if you had nodegraphs with float, color3, vector3 etc outputs. Then you'd want consistent version, nodegroup, namespace, etc but the signature would
differ slightly depending on the nodegraph. e.g. You could end up with:
myspace:ND_foo_v1_float
myspace:ND_foo_v1_vector3
myspace:ND_ foo_v1_color3
etc...
There was a problem hiding this comment.
@kwokcb I'm not quite sure what you mean by "someone does this and changes the version".
Since the desired version is being stated just once by the client, with no coupling between the NodeGraph and NodeDef, I don't see the benefit in stating this version inside of a DefinitionOptions class instead of stating it explicitly through a setVersion call.
What is the situation you're imagining, where using the DefinitionOptions structure would be more robust to version changes?
There was a problem hiding this comment.
@kwokcb I'm not quite sure what you mean by "someone does this and changes the version".
Since the desired version is being stated just once by the client, with no coupling between the NodeGraph and NodeDef, I don't see the benefit in stating this version inside of a
DefinitionOptionsclass instead of stating it explicitly through asetVersioncall.What is the situation you're imagining, where using the
DefinitionOptionsstructure would be more robust to version changes?
The main issue with the API in general is that users must somehow find out what attributes to set -- mostly by reading the spec. So the options make the inputs explicit and as part of the createDefinition() API -- atomic. Done separately implies that it can be done at any time which should not be the case. True you can't stop folks from making attribute setting calls but I'd think it would have to be a intentional decision to find the attribute and set it.
Maybe the over-arching idea behind explicitly listing options is knowledge and safety (at least at creation time), then ease-of-use.
There was a problem hiding this comment.
I see where you're coming from, but I'm not quite as convinced that the DefinitionOptions structure adds clarity or robustness from the user's perspective. To my mind it's just hiding the details of what happens inside of createDefinition, and it seems clearer and more robust for the client to see these explicit steps in their code. My hope is that createDefinition would handle all of the details that are implied by the input NodeGraph itself (e.g. name, category), but not try to handle details that the client needs to explicitly define (e.g. versioning, documentation, and so forth).
There was a problem hiding this comment.
Also my thinking is the emphasis of the API is on nodeef creation so you want the arguments available with a nodegraph is one source which affects the nodedef's characteristics, not so much that you create "some" nodedef with a nodegraph and then specify it's characteristics afterwards.
I think the flip-side question is how to create consistent definitions if the code logic is explicitly performed in separate steps. For example for "global" consistency the utility below exists. Having all args allows a way to take all the information to produce unique ids which are also used for Element existence checking. It would be difficult to promote this if examples show attributes being added after the definition creation.
string generateIdentifier() const
{
if (!compoundGraph || categoryString.empty())
{
return EMPTY_STRING;
}
std::string identifier = categoryString;
if (useVersion && !versionString.empty())
{
identifier += "_" + versionString;
}
for (OutputPtr output : compoundGraph->getOutputs())
{
const std::string& outputType = output->getType();
identifier += "_" + outputType;
}
return identifier;
}
For:
namespace: This needs to be set on the nodedef and nodegraph so the simple reasoning would be to just avoid mistakes.
nodegroup: can be set after, but can change the definitions codegen behaviour, and as far as I've seen nobody thinks about this so will likely be forgotten. So folks can end up with unexpected shader gen results. For example
the houdini nodes are in the houdini group but if they were to add pbr nodes then the behaviour could be unknown.
category, compound nodegraph, & unique signatures for the nodedef and functional nodegraph should be consistent -- and this relies on the identifier generation.
document agreed can be done anytime, but again folks don't seem to think about adding it in, so most nodes are undocumented.
I just don't see anyone ever remembering this or they do how can you promote / enforce producing consistent definitions.
BTW the existing addNodeDefFromGraph() already has most of the "desired" requirements. Perhaps we just keep this
with the logic fixes and add in the unique id generator as a util or embedded inside the API for id generation consistency :). The only outlier is namespace which could be added for the aforementioned reason that I don't think
anyone will remember to set it on the graph and nodedef. Which I think led to the struct creation...
There was a problem hiding this comment.
Omitting these changes to addNodeDefFromGraph sounds like a good strategy to me, and from a high level I would recommend that you make this pull request as minimal as it can possibly be. That should definitely help me to provide a useful review with my limited GitHub development time, and hopefully it will make this change easier to maintain in the future as well. :)
There was a problem hiding this comment.
Hi @jstone-lucasfilm
I've created 2 smaller PRs. The original one which was just to update the API logic (no signature change) is #1403.
The UI integration into the graph editor is here #1404.
I will close this one out once I copy over all the PR docs.
API Logic Updates
This s a set of fixes to the existing "publishing" / interface API to fix logic errors and conform properly with the current specification. The current interface
addNodeDefFromGraph()has been deprecated and replaced with a newcreateDefinition()interface which takes one argument which contains all possible options.<nodedef>Updates<nodedefs>has logic errors in it so adding this PR to fix it up and to make sure it matches the current specification. Of note was that inputs were not being removed from functional graphs, inputs were not being published, and attributes which are disallowed on functional graphs or nodedefs were not being filtered out.<nodegraph>Updates<nodegraph>(vs functional) support so add it in so input interfaces can be "published" from nodes for both compound and functional graphs.General
xpos,ypos) which is generated by theMaterialXNodeEditorandsourceURI.interfacename.Integration
categoryand a compound node graph is required with all other arguments being optional.Publishing_V1.mp4
createdefinition.pyhas been added which will use the current logic if the version is 1.38.8, otherwisecontains the logic fixes as part of the script. Example syntax:
Unit Test Update
The publishing and definition creation test has been refactored to test the logic changes.
Input Data
MaterialXNodeEditorExample 1 : Definition from Nodegraph
Example 2 : Exposing functional graph inputs