-
Notifications
You must be signed in to change notification settings - Fork 342
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
Definition and NodeGraph Publishing Logic Updates #1303
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. |
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've written up some thoughts on the new use case for createDefinition
in the graph editor, since that's a great example to focus on.
options.newNodeDefName = newNodeDefName; | ||
options.newNodeGraphName = newGraphName; | ||
|
||
mx::NodeDefPtr nodeDef = doc->createDefinition(options); |
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 asetVersion
call.What is the situation you're imagining, where using the
DefinitionOptions
structure 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theMaterialXNodeEditor
andsourceURI
.interfacename
.Integration
category
and a compound node graph is required with all other arguments being optional.Publishing_V1.mp4
createdefinition.py
has 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
MaterialXNodeEditor
Example 1 : Definition from Nodegraph
Example 2 : Exposing functional graph inputs