Skip to content

Conversation

@AdvanceControl
Copy link
Contributor

Fixes #112223

At runtime, the engine will rename node names that contain invalid characters.
This PR adds a warning to be logged in that case to avoid user confusion.

@AdvanceControl AdvanceControl requested a review from a team as a code owner November 2, 2025 06:04
@Chaosus Chaosus added this to the 4.6 milestone Nov 2, 2025
Copy link
Member

@Ivorforce Ivorforce left a comment

Choose a reason for hiding this comment

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

Makes sense to me. I'd try to stay close to the existing error message (from #112223), so I'd add some more context.

@Ivorforce Ivorforce requested a review from a team November 2, 2025 23:14
@AdvanceControl AdvanceControl force-pushed the add_warn_when_renamed_node branch from 5cbd4f6 to 0d46d42 Compare November 3, 2025 09:59
@Repiteo Repiteo merged commit 2ac27b2 into godotengine:master Nov 4, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 4, 2025

Thanks!

@harrisyu
Copy link
Contributor

harrisyu commented Nov 5, 2025

This PR conflict with

void Node::_validate_child_name(Node *p_child, bool p_force_human_readable) {

_validate_child_name use '@'.

@AdvanceControl AdvanceControl deleted the add_warn_when_renamed_node branch November 5, 2025 04:54
@Ivorforce
Copy link
Member

What do you mean by "it conflicts" with the code?

@harrisyu
Copy link
Contributor

harrisyu commented Nov 5, 2025

What do you mean by "it conflicts" with the code?

I mean:
When a child node was added to parent_node with add_child, parent node will check it's name with _validate_child_name,
and invalid char or duplicated name with replace with @ char , then it will trigger this warning, sometimes alot, and it's not trackable through these messages, especially in editor code. _validate_child_name function need to modify too.

@AThousandShips
Copy link
Member

How is this warning triggered? set_name isn't called anywhere in add_child as far as I can see?

@harrisyu
Copy link
Contributor

harrisyu commented Nov 5, 2025

How is this warning triggered? set_name isn't called anywhere in add_child as far as I can see?

There is no detail infomation in these warnings, so it's hard to tell where is it come from.
And it's hard to reproduce, I got some label name warning maybe cause by some addon creating list of labels with same default name, But after I changed @ to _ in _validate_child_name in my local build, no more warnings.

BTW, since we restrict on invalid node name characters, I think they should not use internal too.

@AThousandShips
Copy link
Member

I'd suggest opening a bug report, but just adding a child doesn't cause the name to be set so this can't be all that's done in this case, I'd say that it might be that you're reassigning the name or something

@akien-mga
Copy link
Member

akien-mga commented Nov 5, 2025

Node.duplicate() calls set_name, and can be used on nodes instantiated from code with a default generated @ name, so this would trigger warnings.

There might be other uses of set_name outside of Node in subclasses. So I think this warning might indeed be raised frequently outside of the users' control.

Random example:

resources/3d/importer_mesh.cpp
588:            mesh->set_name(get_name());

3d/mesh_instance_3d.cpp
259:    static_body->set_name(String(get_name()) + "_col");
289:    static_body->set_name(String(get_name()) + "_col");
328:    static_body->set_name(String(get_name()) + "_col");

@Mickeon
Copy link
Member

Mickeon commented Nov 5, 2025

I haven't tested any build with this PR, but the above concern is valid in my opinion.
I think the review process of this PR was a bit too hasty. The lack of a warning prior to this PR makes me think some Godot developers, and even Godot's codebase, may be using this behavior on purpose without calling String.validate_node_name() beforehand. That, plus the engine already validates Node names every time they are set, so calling that method manually is wasteful.

I think that should've been considered before merging this PR and I would personally revert this.

@Repiteo Repiteo changed the title Add warning when node name is invalid Add warning when node name is invalid (reverted) Nov 5, 2025
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.

Invalid node names don't trigger an error during runtime

8 participants