-
-
Notifications
You must be signed in to change notification settings - Fork 21.1k
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
Add Node Existence Toggle #92377
base: master
Are you sure you want to change the base?
Add Node Existence Toggle #92377
Conversation
In terms of UX, this is another icon in a place that is already crowded with icons. A node can have a whopping 5 icons when it's in a group, has a connected signal, has a script, is visible, and now is enabled. I don't have a solution for this now, other than that maybe there should probably be options for which icons are visible there. Maybe a 'show icons' dropdown at the top of the scene tree window? |
Hey, I like it I have some questions:
Edit: if the solution to (1) is to combine both icons into one it will also address the issue that @AdriaandeJongh raised |
Would really like to see this go in or something similar. I've been wanting an easy way to test scenes with nodes on/off without having to completely destroy it and lose all data, or make any editor plugin hacks to work around it. |
I was thinking about UX problem @AdriaandeJongh mentioned. One solution I can think of is having 3-state visibility button - visible, hidden, disabled (as disabled is hidden at the same time). Eye icon in hidden state can be filled half-way to illustrate that there is yet another click possible. And to go back to visible state again you would have to cycle through all states. |
Does the icon really need to be in the SceneTree for quick access? I can imagine it is initially enough to having it in the inspector together with the Node properties. If then enough people use the feature and want 'quick access' to it, then a followup PR could add the icon. |
See also #56446 |
How does this close godotengine/godot-proposals#3433? I don't see any editor only or game only options for this. Nothing that automatically does this when in editor and not, rather than using scripts to do that etc. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@LauraWebdev Please don't bump without contributing significant new information. Use the 👍 reaction button on the first post instead. |
@AdriaandeJongh yep, like mentioned in the post this could alternatively be hidden under the right-click menu to reduce the clutter. personally, i prefer it immediately accessible (for prototyping, etc.). I don't have any strong opinions one way or the other, this was simply what I ended up preferring.
@AThousandShips By disabling existence and not visibility, a node remains visible and active in the editor but not in the built project. This is done automatically. However, this doesn't implement the inverse behavior (the node exists in the built project but not in the editor). So I don't believe this fully closes that proposal. I've edited the main post. |
@torcado194 What about renaming "Toggle Existence" to "Toggle Editor-Only" to make it unambiguous? |
@passivestar I think that's fine, yeah. It didn't register to me that that is what this is ultimately doing. Alternatively, I've considered the idea of a 3-state toggle: "editor-only", "game-only", and "disabled". This would cover all the current use cases and also end up closing godotengine/godot-proposals#3433. Maybe should be a separate PR, but I do think I want to implement this for myself now haha |
What if:
This would make both workflows one-click |
Toggling visibility and toggling whether a node is editor-only are two wholly different concepts to me. On top of that and unlike toggling visibility, making a node editor only has a lot of far reaching consequences such as non-editor-only nodes not being able to reference them. Such consequences make me vote strongly against putting it under / alongside the visibility toggle. I like the feature / idea a lot, but my suggestion would be to put the option to make a node editor-only under the Node category, alongside the fields for process and metadata and such. |
@AdriaandeJongh I think those points are fair. I feel like these different solutions are circularly conflicting (having a button on the node tree allows for quick prototyping but adds clutter, removing the button but adding a shortcut to toggle it removes the clutter but semantically couples "editor-only" and "visibility" when they should be distinct, moving the button to the inspector decouples the properties but removes the quick toggle). I prefer some way to quickly toggle the state because prototyping is my main use case for this feature, but if the button were moved to the inspector panel it wouldn't really matter to me, i'd just make a plugin to add a hotkey/button to switch it anyway. Whatever the consensus is, I'm happy to make changes :) |
I think having this function in the context menu and exposed to editor's shortcuts to be able to bind some hotkey to it (can be empty by default) is the best compromise here. Then you don't need to make an addon |
Let's discuss the more general details of the feature on the proposal, and strictly the implementation details here |
@torcado194 For me the red text indicates that there's an "issue" with the node so I personally would avoid that just because for me it indicates that I need to check it to see if something is broken, like a missing resource reference or something. I do think it needs some kind of visual that clearly shows it's somehow different than other nodes without looking like the node is somehow broken or has an error. If the icon has multiple states, I think it would make sense to change the icon itself as well as the hover tooltip. So for example changing the eye to a dark grey E that says "Editor-Only" on hover or something similar, while the eye icons continue to say "Toggle Visibility" on hover.
@passivestar Adding a shortcut to speed up toggling it would definitely be nice 100%, my only thing would be that the functionality would be hidden behind a shortcut and the information for it isn't surfaced anywhere in the editor itself which I think is a valuable even if there is a shortcut. I think that would be solved by @AdriaandeJongh's suggestion of putting it in the Inspector with the Process drop down, a tooltip could be put there saying something like "This can also be toggled by Ctrl+Clicking the Visibility toggle icon in the scene view" or something.
@AdriaandeJongh I personally like having it in the scene tree because being able to tell what is set to editor-only is useful to be able to at a glance see what won't be added to the build. Putting the toggle in the inspector view with process would be fine, but I would still have some kind of visual indicator to show that it is editor-only in the scene view. |
This is absolutely needed, but the whole icon debate is making another problem surface: There's already a LOT of icon clutter and it's a creeping problem, so one thing that does come to mind is to have the visibility toggle house node disabling functionality. Basically, you would cicle from visible to hidden to disabled, then back to visible. Maybe eye, closed eye and forbidden symbol. My reasoning is that I don't see how anyone would need to make a node invisible if they are going to also disable it. Double clicking MIGHT get annoying, so maybe other mouse buttons could cicle backwards if used on top of the button. I know I'm glossing over the disabling scope nuance (editor only, game only) but I feel it might be justifiable for this to be buried just under the surface instead of a button on the tree. After all this would be a very specific and deliberate technical choice, and you really don't want to set stuff to editor only on accident and then be confused. To that extent a node set to be editor only could rock a "G" within a forbidden sign instead of the plain forbidden sign with nothing inside (which means disabled across the board) |
I would go with a dropdown menu at the top of the inspector with Enabled/Disabled/Editor-only (RMB context menu is also fine but less discoverable), not adding a clickable icon, and signifying state in the scene tree view with graying out the name for disabled and coloring it for editor-only (I think yellow is taken, so maybe blue?) |
4b996cd
to
13dfb5d
Compare
564722f
to
e508fda
Compare
I have made changes following the suggestions made here and in the linked proposal.
I think it also makes sense to rename this PR to "Add Editor-Only Toggle", let me know if there's any reason I shouldn't do that. Here is a demo of the changes: |
e508fda
to
b386e47
Compare
I fear using color to convey information, SPECIALLY blue is a big no-no for accesible design. I have no idea how much control over the text formatting on node's names is exposed, but wouldn't bold, italics, crossed-through or even color flipped presentation (see image) be more prudent? On a side note, maybe it's just me, but back when I used Unity I did some stuff by having their equivalent of nodes disabled full stop and re-enabling them at runtime, so settling with just editor only disabling does narrow possibilities a bit. |
To my knowledge, you can't use different font styles for individual tree items, but you can give them a dedicated background color. This is already done in the FileSystem dock for colored folders. |
Technically you can use custom draw for that and draw the text manually. |
Just curious, is hijacking the eye button like I suggested really far out there and off the table? |
Look at godotengine/godot-proposals#7715 and godotengine/godot-proposals#3433 for context and more theoretical discussion. Edit: Links to proposal tracker |
Oh, blue is already used for editor-running script icons, did not notice that. I guess it either makes blue a better candidate for editor-only nodes from this patch, or if it is rejected due to accessibility then the editor-running script icons should be looked at as well (they could have a T letter on the icon for example). |
@PLyczkowski it's not blue it's the accent color: |
Oh. Not sure that's a good implementation then, no way to remember 'tool scripts look like this'... Icon with added gear or the letter T would be better imo |
Sorry for the delay. I love the idea of a node existence toggle, but I don't really like the fact that it's closely binded to the editor. |
As I understand it, this proposal gets the job done by doing some hacky procedures to get nodes to actually cease to exist. The moment disabling becomes an official clean feature, like a boolean you can set, both code use and gui implementation will be easier. |
After a quick read of the code changes, what I've got from it is that it adds a new value to the base node class that when set makes it so the routine to instantiate a packed scene doesn't actually instantiate the nodes marked as "editor-only" if the scene is being loaded in a build.
I think adding code features could be added later, simply because adding the "editor-only" flag at runtime would not have any effect since this affects the Packed Scene loading routine. So making it accessible via code would only add the ability for tool scripts to set the flag in the editor, and while that could be useful, I don't think it should be a blocker. The other proposal that is discussing "enable/disable" is different. Since the node is loaded but can be "enabled/disabled" during runtime I think it makes way more sense that it's accessible via code: The only other additional code driven enhancement might also be an additional flag in GDScript like |
b386e47
to
6a723fd
Compare
Closes godotengine/godot-proposals#7715
Related: godotengine/godot-proposals#3433
This PR adds an additional toggle to nodes in the tree, allowing you to toggle the existence of the node when running or exporting the project. This allows for editor-only nodes and completely removed nodes, when combined with the existing visibility toggle.
This is one of the first changes I made to Godot locally, about a year ago. I find it invaluable for prototyping. I've since given the code to friends and anyone who's asked for it. I figured enough people have wanted it that I should submit a PR for it.
(This could alternatively be hidden behind the right-click menu. I opted for this solution since I primarily use it for quick prototyping, so having immediate access is preferred.)
Note about the implementation:
I originally implemented this as an additional
tag
on the node resource, so that when parsing the node inresource_format_text.cpp
it could apply thedisabled
state directly so thatSceneState::instantiate()
could read that data up front rather than needing to parse the properties. The metadata implementation was used instead because I did not want to interfere with the binary saving/loading system, which is necessary for keeping the behavior when exporting. I still believe the tag is the better solution, if anyone has insights about this I would appreciate it.