Error "Invalid node name for submenu, ..." on project load in editor (mono).

[Shilo]

Godot version

v4.2.beta5.mono.official [4c96e96]

System information

Windows 11 Home 64-bit - Godot v4.2.beta5.mono

Issue description

Error:

Invalid node name for submenu, the following characters are not allowed:
. : @ / " %

This only happens in the mono version for all projects. This is also a regression bug and doesn't happen in v4.2.beta4.mono.

The high level cause of the bug is the Project > Tools > C# submenu that fails to load.

The low level cause is that the "C#" submenu PopupMenu doesn't have a validate node name (or any explicit node name in general). The name defaults to @PopupMenu@17128 and the validated name becomes _PopupMenu_17128.

The error is due to the addition of validation in this commit:
#84183
137b25c

I would fix it directly and make a pull request if I knew the proper solution. It seems the node name validation is inconsistent with the "default placeholder node names".

Possible fixes:

• Give the PopupMenu an explicit node name at modules\mono\editor\GodotTools\GodotTools\GodotSharpEditor.cs line 501
  Example:

_menuPopup = new PopupMenu();
_menuPopup.Name = "CSharp";

• Rethink the validation in the aboved mention change: Fix activate_item_by_event infinite recursion crash #84183

(Note: New to bug reporting, please let me know if I could have made it more proper.)

Steps to reproduce

• Open or create any project in v4.2.beta5.mono.official [4c96e96]
• Read output log

Minimal reproduction project

Empty Godot project.
Empty.zip

[Shilo]

Sorry failed to find the bug was already mentioned here: #84572

[AThousandShips]

The error is due to the addition of validation in this commit

The error predates this, that PR just exposed it

[YuriSizov]

I would fix it directly and make a pull request if I knew the proper solution. It seems the node name validation is inconsistent with the "default placeholder node names".

What you suggest are indeed two possible solutions. An argument can be made that we should support unnamed nodes by allowing @ into this particular validation. But for now I think the solution should be as simple as naming the node. Note that the name should follow the codestyle, so it would be something like CSharpTools.

[Shilo]

I would fix it directly and make a pull request if I knew the proper solution. It seems the node name validation is inconsistent with the "default placeholder node names".

What you suggest are indeed two possible solutions. An argument can be made that we should support unnamed nodes by allowing @ into this particular validation. But for now I think the solution should be as simple as naming the node. Note that the name should follow the codestyle, so it would be something like CSharpTools.

Great points. this was my thought process too as I think it's best to allow unnamed/default named nodes as that's many areas of the editor node. The submenu names are also inconsistent as I printed them out and tried to follow a style that was most common in which I saw snake_case. There was also CamelCase and Title Names. But im not entirely familiar with Godot's standards and workflow.

[YuriSizov]

The submenu names are also inconsistent as I printed them out and tried to follow a style that was most common in which I saw snake_case. There was also CamelCase and Title Names. But im not entirely familiar with Godot's standards and workflow.

Which menus did you inspect? PascalCase for node names is the recommended style for all Godot projects, and the editor is no exception. Odd inconsistencies are possible, but should likely be fixed.

https://docs.godotengine.org/en/stable/tutorials/best_practices/project_organization.html#style-guide

[Shilo]

Which menus did you inspect?

The ones that are inconsistent seem to be the following:

• Label: Run Multiple Instances = Name: run_instances
• Label: Display Advanced... = Name: display_advanced
• Label: Submenu = Name: submenu
• Label: Remove Branch = Name: Remove Branch
• Label: Remove Remote = Name: Remove Remote
• Label: C# = Name: @PopupMenu@17128

There may be more as I didn't inspect them all.

[YuriSizov]

Yeah, all those can be fixed. If you plan to do a PR for the C# submenu issue, feel free to adjust those as well.

[YuriSizov]

@Shilo Sorry, this is a bit time sensitive now, so I fixed it myself.

[Shilo]

@Shilo Sorry, this is a bit time sensitive now, so I fixed it myself.

No problem. I thought it was clear that I prefered to avoid fixing because I think the proper route is fixing the validation route for unnamed nodes (as well as naming the submenus), in which case I wasn't sure what the approach would be. I guess the pull request was just the naming change though.

Thanks for your hard work btw!