Add new token node (adsk) + supporting logic. #1204

bernardkwok · 2021-05-17T20:13:07Z

Token node and token connection ground work.
Fix for string resolver token addition to include looking a all parent and sibling tokens.

New token node definition

  <nodedef name="ND_token_node" node="token_node" nodegroup="organization" namespace="adsk">
    <token name="token1" type="string" value="" />
    <token name="token2" type="string" value=""  />
    <token name="token3" type="string" value=""  />
    <token name="token4" type="string" value=""  />
    <token name="token5" type="string" value=""  />
  </nodedef>

Tests

New test configuration:

 <!-- Sample graph using with nodes and token node member -->
   <nodegraph name="token_node_graph" fileprefix="resources/Images/" xpos="-7.2995" ypos="123.242" >
    <token name="name" type="string" value="grid" />
    <token name="extension" type="string" value="png" />
    <adsk:token_node name="token_node" type="" nodedef="adsk:ND_token_node" xpos="-128" ypos="-367">
      <token name="token1" type="string" interfacename="extension" value="png" />
      <token name="token2" type="string" interfacename="name" value="grid" />
    </adsk:token_node>
    <image name="image" type="color3" nodedef="ND_image_color3" xpos="-127.5" ypos="-78">
      <input name="file" type="filename" uniform="true" value="[name].[extension]" />
    </image>
    <output name="grid_out_via_token_node" type="color3" nodename="image" />
  </nodegraph>

which looks like this graphically:

The compound graph being used as the functional graph for a nodedef:

  <!-- Sample graph as a definition -->
  <nodedef name="ND_tokenized_image_node" node="token_image_node"  >
   <token name="name" type="string" value="ND_grid" />
   <token name="extension" type="string" value="ND_png" />
   <input name="file" type="filename" uniform="true" value="[name].[extension]" />
   <output name="grid_out_via_token_node" type="color3" value="0, 0, 0" />
  </nodedef>
  <nodegraph name="NG_tokenized_image_node" nodedef="ND_tokenized_image_node">
   <token name="name" type="string" value="NG_grid" />
   <token name="extension" type="string" value="NG_png" />
   <input name="file" type="filename" uniform="true" value="[name].[extension]" />
   <adsk:token_node name="token_node" type="" nodedef="adsk:ND_token_node" xpos="-128" ypos="-367">
      <token name="token1" type="string" interfacename="extension" value="N_png" />
      <token name="token2" type="string" interfacename="name" value="N_grid" />
   </adsk:token_node>
   <image name="image" type="color3" nodedef="ND_image_color3" xpos="-127.5" ypos="-78">
      <input name="file" type="filename" uniform="true" interfacename="file" />
   </image>
   <output name="grid_out_via_token_node" type="color3" nodename="image" />
  </nodegraph>

And a sample instance usage:

   <!-- Definition instance usage -->
   <nodegraph name="token_image_graph" fileprefix="resources/Images/" >
      <token_image_node name="token_image_node1" type="color3" nodedef="ND_tokenized_image_node" >
         <token name="name" type="string" value="cloth" />
         <token name="extension" type="string" value="png" />
         <input name="file" type="filename" uniform="true" value="[name].[extension]" />
      </token_image_node>
    <output name="token_image_node1_out" type="color3" nodename="token_image_node1" /> 
   </nodegraph>

Results

Rendered results:

Note that the resolved value is "grid.png" for nodegraph example
and "cloth.png" for the instance of the defintion

Unit test results:
token_tests.pdf

- Fix for string resolver token addition to include looking a all parent and sibling tokens.

Fix up test.

bernardkwok · 2021-05-18T00:00:44Z

source/MaterialXRuntime/Private/Commands/PvtConnectionCmd.cpp

+void PvtInterfaceConnectionCmd::updateConnectionProperties(const RtOutput& src, const RtInput& dest, RtCommandResult& /*result*/)
+{
+    // Copy token information over
+    if (dest.isToken())


@niklasharrysson , I made this port update a virtualization. As with discussed with Jonathan before this would not be required if we could set this at port creation time but we don't have enough information (only name + type is passed).

I imagined PvtInterfaceConnectionCmd being implemented differently. This is the public API I prosed in our earlier discussion:

/// Connect a node input to the parent graph's input interface. The interface socket will be created /// on the graph if it doesn't exists already. void makeInterfaceConnection(const RtInput& nodeInput, const RString& graphInputName, RtCommandResult& result); /// Connect a node output to the parent graph's output interface. The interface socket will be created /// on the graph if it doesn't exists already. void makeInterfaceConnection(const RtOutput& nodeOutput, const RtString& graphOutputName, RtCommandResult& result);

The socket on the nodegraph (here only given my name) would be created if it doesn't exists, and all information would be available internally to create it with the same flags as the node input it is to be connected to.

I could do it this way, but doesn't this repeat existing connection code ?
Also, the way things work currently is that ports are created first so it is assumed that it exsts but could add creation on demand but unsure what will occur as it could mismatch with what is created in VNN.

It could still be derived from PvtConnectionCmd and use the same methods for making/breaking connections. Just that in addition it creates the interface input first if it doesn't exists, and with the correct flags set.

From a pure runtime API perspective it makes perfect sense to have such a command that encapsulates these operations into a single command, since it's a common task an API user would need.

But I understand in LookdevX it's a problem of also having to follow the rules of how VNN operates. If the interface input is created first in VNN it might be better to just use the normal connection command and then work around it in LookdevX with low-level API (setting the flags after). I didn't consider this VNN issue when proposing we create a new command for this.

Yes. I should have been prefaced that there is no way to create the correct port properties when it's dynamically created via VNN. There is still a "hack" outside the command code which copies over value data as well but I guess can only be done when the connection part is done (after creation). It lives outside the runtime command since it does it by setting it on the VNN port and then calling another runtime command which sets the value.

Maybe it's worth to try another go at finding some kind of proper callback from VNN to allow proper port creation.

bernardkwok · 2021-05-18T00:01:08Z

source/MaterialXRuntime/Private/PvtPort.cpp

-    {
-        return false;
-    }
+    //if (isToken() || output->isToken())


Forgot to clean this. Will delete.

bernardkwok · 2021-05-18T00:01:41Z

source/MaterialXCore/Element.cpp

@@ -548,7 +548,7 @@ bool ValueElement::validate(string* message) const
    }
    if (hasInterfaceName())
    {
-        validateRequire(isA<Input>(), res, message, "Only input elements support interface names");
+        validateRequire(isA<Input>() || isA<Token>(), res, message, "Only input and token elements support interface names");


Allow interface exposure for tokens.

bernardkwok · 2021-05-18T00:02:03Z

libraries/adsk/adsklib/adsklib_defs.mtlx

+    Node: token_node
+    Non-rendering node used to hold a set of token values
+  -->
+  <nodedef name="ND_token_node" node="token_node" nodegroup="organization" namespace="adsk">


adsk only for now.

I'm not sure why this node is needed? It seems it's a node just to plug tokens into, and there are not outputs and it's never used anywhere. Maybe you could give some more context why it's needed?

Yes. This is the proposal to have a reusable node type which can hold a set of tokens.
It's more of a user workflow way to take a set of tokens and connect them up to as interfaces on nodegraphs and have it serializable.

There is no data flow per-se.

Do you mean it's a way for the user to create tokens on a nodegraph interface, by creating a token_node and connecting its inputs/tokens to the nodegraphs interface to have them created there?

Because once created the tokens are part of the nodegraph interface, and their value is saved there. So after creation the token_node is no longer needed, right?

If this is the case it seems like it's a workaround for a limitation in the UI. Would it not be better so solve this in the UI instead and add support for creating/removing/editing tokens directly on a nodegraph's interface?

If it's a temporary workaround until VNN has this supported that's totally fine, but I'm not sure it's a good long term solution for creating tokens on an interface. Just let me know if I missed something and there is more to this than just a workaround for the UI.

Hope this helps for clarity:

The base assumed functionality is tokens on a nodegraph which get applied withing the scope of the nodegraph.
As with inputs, tokens are made part of the interface and hence can have variants (presets) applied to them.

The intent of the token node is keep around reusable set of tokens to apply to only within nodegraph. i.e. they are locally scoped.
2a. The initial logic was to that just the existence of the token node within a nodegraph meant to apply all the tokens.
2b. The current variant is to allow connections which means that to only use tokens from token node which have connections. Basically allow for selective reuse of tokens from a token node.

Direct creation of tokens on a nodegraph isn't currently a UI requirement (and also not possible in VNN), though is possible without UI. (Fixes for that we're in the previous check-in).

@bernardkwok Thanks for the additional details. I understand the intent better now.

The fact that existence of such a node overrides the tokens set on the nodegraph is a new concept I think. And I suppose this has to be cleared with Lucasfilm later as a proposal for the spec?

I find it a little bit peculiar to have a local override inside a nodegraph that overrides something set outside (if a token is set on the nodegraph interface but also in a token_node internally). Just my 2 cents, and don't let this block the code review if you want to merge this in for now to work on it further.

The proposal I put up a little while ago and awaiting feedback.

I will try and provide more in the description and ping again. In the meantime, the unconnected case w/o a node I'll probably seperate out into a different check-in as those are fixes for already supported functionality.

bernardkwok · 2021-05-18T00:03:50Z

source/MaterialXCore/Interface.cpp

-        interfaceInput->addTokens(resolver);
-    }
-    else
+    ConstNodeGraphPtr graph = getAncestorOfType<NodeGraph>();


Generalize as the parent graph could have tokens which are not associated with an interface input.
Also include checking for tokens at the input level.

niklasharrysson · 2021-05-18T14:27:24Z

source/MaterialXGenShader/Util.cpp

+    if (nodeDef->getNodeGroup() == ORGANIZATION_STRING)
+    {
+        return false;
+    }
    static string TYPE_NONE("none");
    const string& typeAttribute = nodeDef->getType();
    return !typeAttribute.empty() && typeAttribute != TYPE_NONE;


Do you really need the new test for "organization" above?

Is this test for type not enough, either having an empty type or type=="none"?

The new node seems to have a type. Let me take a look. Perhaps I need to specifically set the type to "none" ?

Ah yes, if no type is set I think MaterialXCore will default to "color3" or something like that.

- Token node and token connection ground work.

6b8e088

- Fix for string resolver token addition to include looking a all parent and sibling tokens.

bernardkwok requested a review from niklasharrysson May 17, 2021 20:13

bernardkwok added 4 commits May 17, 2021 17:07

Add in "dummy" impl to avoid Runtime erroneous missing impl error.

26913d5

Fix up test.

Remove unneeded impl.

1fd71ea

Test fixes.

a6fd267

Fixes.

bccd201

bernardkwok changed the title ~~Add new token node (adsk only for now) + supporting logic.~~ Add new token node (adsk) + supporting logic. May 17, 2021

bernardkwok commented May 18, 2021

View reviewed changes

bernardkwok requested review from ashwinbhat and nicolassavva-autodesk May 18, 2021 12:18

niklasharrysson reviewed May 18, 2021

View reviewed changes

bernardkwok requested review from jstone-lucasfilm and removed request for nicolassavva-autodesk June 1, 2021 13:25

bernardkwok added the Do Not Merge This PR should not be merged label Jun 9, 2021

Merge.

59b0462

bernardkwok closed this Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new token node (adsk) + supporting logic. #1204

Add new token node (adsk) + supporting logic. #1204

bernardkwok commented May 17, 2021 •

edited

Loading

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

bernardkwok May 18, 2021

bernardkwok May 18, 2021

bernardkwok May 18, 2021

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

bernardkwok May 18, 2021

niklasharrysson May 19, 2021

bernardkwok May 19, 2021

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

bernardkwok May 18, 2021

niklasharrysson May 18, 2021

Add new token node (adsk) + supporting logic. #1204

Add new token node (adsk) + supporting logic. #1204

Conversation

bernardkwok commented May 17, 2021 • edited Loading

New token node definition

Tests

Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bernardkwok commented May 17, 2021 •

edited

Loading