Skip to content

Add setNameGlobal which updates connections downstream#2386

Merged
jstone-lucasfilm merged 11 commits into
AcademySoftwareFoundation:mainfrom
rasmustilljander:add_set_name_function_node_nodegraph
Jun 8, 2025
Merged

Add setNameGlobal which updates connections downstream#2386
jstone-lucasfilm merged 11 commits into
AcademySoftwareFoundation:mainfrom
rasmustilljander:add_set_name_function_node_nodegraph

Conversation

@rasmustilljander

@rasmustilljander rasmustilljander commented May 15, 2025

Copy link
Copy Markdown
Contributor

This solves (or partially solves): #1847

This commit adds the method setNameGlobal for Node and NodeGraph. It behaves like setName but also walk downstream and updates corresponding node or nodegraph name.

API added

void Node::setNameGlobal(const std::string& name);
void NodeGraph::setNameGlobal(const std::string& name);

Comment thread source/MaterialXCore/Node.cpp Outdated

@ld-kerley ld-kerley left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @rasmustilljander - this looks like it's gonna make editing documents a lot more straight forwards!

Comment thread source/MaterialXCore/Node.cpp Outdated

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very promising, @rasmustilljander, and I've written up a suggested improvement.

Comment thread source/MaterialXCore/Node.h Outdated
@rasmustilljander rasmustilljander changed the title Add setName with updateReferences Add setNameGlobal which updates connections downstream May 26, 2025

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, @rasmustilljander, and I've written up a proposal for the language of the doc strings.

Comment thread source/MaterialXCore/Node.h Outdated
/// @name Name
/// @{

/// Set the element's name string. The name of a MaterialX element must be

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these new docstrings, my recommendation would be to be specific about the element subclass (e.g. Node, NodeGraph), and to omit the repeated language of the general rules for MaterialX element names.

Here's a proposed update to the doc string for the Node method, and the same approach should work for NodeGraph as well:

/// Set the name string of this Node, propagating the updated name to all
/// downstream ports.
/// @throws Exception if an element at the same scope already possesses the
///     given name.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, I updated the docs to align with your suggestion!

rasmustilljander and others added 6 commits June 2, 2025 09:36
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>
Signed-off-by: Jonathan Stone <jstone@lucasfilm.com>

@jstone-lucasfilm jstone-lucasfilm left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, thanks @rasmustilljander!

@jstone-lucasfilm jstone-lucasfilm merged commit f564898 into AcademySoftwareFoundation:main Jun 8, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants