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

Node inputs still keep a reference to removed nodes. #419

Closed
alister-chowdhury opened this issue May 5, 2020 · 3 comments
Closed

Node inputs still keep a reference to removed nodes. #419

alister-chowdhury opened this issue May 5, 2020 · 3 comments

Comments

@alister-chowdhury
Copy link
Contributor

Hey there, this is more of a question.

But in the following example:

import MaterialX

doc = MaterialX.createDocument()
graph = doc.addNodeGraph()

add1 = graph.addNode("add")
add2 = graph.addNode("add")
add2.setConnectedNode("in1", add1)

graph.removeNode(add1.getName())
print(MaterialX.writeToXmlString(doc))

The output will be:

<?xml version="1.0"?>
<materialx version="1.37">
  <nodegraph name="nodegraph1">
    <add name="node2" type="color3">
      <input name="in1" type="color3" nodename="node1" />
    </add>
  </nodegraph>
</materialx>

As you can see, there is still a reference to "node1", which as a node, no longer exists.

I'm not sure if this is a bug or not, but if It's not, is the intention suppose to be that you can now create a new "node1" which will neatly fit in its place?

@jstone-lucasfilm
Copy link
Member

Your description is exactly right, and creating a new node named node1 will neatly replace the original, restoring the connection between node1 and node2.

From a high level, the philosophy of the MaterialX API is that all document edits are local, so modifying or deleting an element in one place (e.g. node1) doesn't edit any other elements in the document. If you'd like to remove the input element from node2, you can delete it in the usual way by calling add2.removeInput('in1').

In a future update, I'd like to add support for calling add2.setConnectedNode('in1', None), which would perform the same work and make a clearer parallel with add2.setConnectedNode('in1', add1).

@alister-chowdhury
Copy link
Contributor Author

Oh I see, thank you for taking the time to explain this, the layering paradigm makes perfect sense.

Is there any general way to remove "ghost" references of this kind (or would that basically be, iterating every node, checking if it has a connection string but no connected node and pruning it at such).
I can imagine some sticky problems arising from a unexpected reference popping back into play, due to a lapse in remembering to break connections.

That's interesting that add2.setConnectedNode('in1', None) doesn't do the equivalent of add2.removeInput('in1') as add2.getInput('in1').setConnectedNode(None) does prune the reference.
Would the intention be that add2.setConnectedNode('in1', None) edits the underlying noderef?

@jstone-lucasfilm
Copy link
Member

Yes, I believe it's just an oversight that add2.setConnectedNode('in1', None) doesn't break the connection, and we'll fix this in an upcoming change. My sense is that this call should actually remove the input element from the node, rather than leaving the input with an empty nodename string.

We don't yet have a high-level method that removes all invalid connections from the document, but this sounds like a good suggestion for a future improvement.

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

No branches or pull requests

2 participants