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

Allow prefixing with '%' to set a node as unique in the Scene Tree Dock #101163

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

Synzorasize
Copy link
Contributor

Closes godotengine/godot-proposals#11386. This allows users to set any node (except for root nodes) as unique while renaming it by simply prefixing with "%". Feel free to request changes.

@Synzorasize Synzorasize requested review from a team as code owners January 5, 2025 22:40
@yankscally
Copy link

so cool! wowee! love me some QoL

@badsectoracula
Copy link
Contributor

badsectoracula commented Jan 6, 2025

I tried this and it works as described. The code looks ok too.

There are two things that i noticed:

  1. Trying to rename something to '%Foo' when there is another unique 'Foo' elsewhere gives some internal warning. Perhaps it could give an error message or produce a new name like if you try to rename something to 'Foo' when there is already an existing 'Foo' (in which case the node is renamed to 'Foo2'). Note that this already happens if you try to rename something to '%Foo' with an existing 'Foo' under the same parent.
  2. I expected that if you try to rename an already unique node, the editable field would add the '%' so you can either remove it, thus making the node non-unique. Technically the PR is about making a node unique, not the reverse, it just feels a bit inconsistent :-P. Also it'd help with just pressing F2 and then Ctrl+C to copy the unique name for use in scripts.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 7, 2025

Trying to rename something to '%Foo' when there is another unique 'Foo' elsewhere gives some internal warning.

The worse part is that this discards your name. Showing warning is probably fine, but the node should still be renamed, just without unique name.

@Synzorasize
Copy link
Contributor Author

Thanks for the suggestions, @badsectoracula and @KoBeWi!

  1. I was going with the original implementation for if there was already a unique name, but I do agree it makes more sense to keep the name if not make it a unique name. I also think renaming the node to "Foo2" makes sense, to ease the workflow further. Since the user would now be able to set a unique name just by renaming the node, it would be the most intuitive and consistent solution here as well. But perhaps someone else can give their opinion about this?
  2. This is pretty similar to @Mickeon's suggestions in Improve how Scene Unique Nodes are highlighted in Scene Tree Editor godot-proposals#4494, which I took a look at. I'm not sure if that would be out of scope for this PR, since it just adds a new feature, not changing any visual appearance. But it can always be implemented in a new PR if we want. For the keyboard shortcut, I was thinking while renaming it, the user could Ctrl/Cmd+A Ctrl/Cmd+C (selecting the entire field and copying) it before confirming so that it can be pasted into code. But I suppose if you want to copy with the "%" even after renaming it, this makes sense.

@Synzorasize
Copy link
Contributor Author

Synzorasize commented Jan 9, 2025

It looks like renaming to "Foo2" would either need some changes in scene/main/node.cpp since it currently only supports renumbering of child nodes, or duplicating the similar logic to the function, which would make it very complex. So that is probably out-of-scope for this PR. But perhaps it can be implemented in a future PR. For now, I will use @KoBeWi's suggestion.

Also fixed a bug where adding "%" without changing the rest of the name while there is already a unique node with the same name does not show a warning.

Edit: Also rebased from master. That's why the comparison between force-pushes way more complex than it should.

@KoBeWi
Copy link
Member

KoBeWi commented Jan 9, 2025

Changing node name to (different) unique one will create 2 undo actions. I think it should be one.

@Synzorasize
Copy link
Contributor Author

Changing node name to (different) unique one will create 2 undo actions. I think it should be one.

This makes sense since only one action was done by the user. I decided to set them as one action.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Jan 13, 2025
@akien-mga akien-mga merged commit 9da7e7d into godotengine:master Jan 13, 2025
20 checks passed
@akien-mga
Copy link
Member

Thanks! And congrats for your first merged Godot contribution 🎉

@Synzorasize Synzorasize deleted the implement_11386 branch January 13, 2025 23:24
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.

Make the node scene-unique if its name begins with % when renaming it in the Scene tree dock
6 participants