godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.1k stars 69 forks source link

Implement support for Node Stashing #10212

Open reduz opened 1 month ago

reduz commented 1 month ago

Describe the project you are working on

Godot

Describe the problem or limitation you are having in your project

Multiple times, when editing scenes in the editor, users would like to (from the editor) set a branch or a node to a disabled state, so when the scene is loaded it does not do absolutely anything (acting as if it was not there). This can be done with the intent of either leaving it disabled (just for debugging), or eventually enable it back under some gameplay condition, as fast as possible, so it starts working normally again.

Currently, to achieve something like this, Godot allows:

While this works to some degree, there are shortcomings with these approaches:

This means, that it is not possible to reach a complete disabled state. Users expect the node to exist (in order to enable it), but to act as if it was not there for the rest of the nodes.

Related proposal: https://github.com/godotengine/godot-proposals/issues/7715

Describe the feature / enhancement and how it helps to overcome the problem or limitation

This proposal is based of Kobewi's idea to allow users to mark nodes as stashed, either in the editor or by code. When a node is marked as stashed, it will load together with the scene but it will not appear in the tree (nor will its sub-children). Instead, it will be stored in its parent node stash.

If the user chooses, they can unstash the node (it gets added back to the tree) and/or stash it again, which happens immediately. Ultimately, stashed or not, the parent node keeps track of the child node and it will erase it properly together with the rest of the children nodes when freed.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Nodes can be stashed or unstashed back by calling the following functions

# On Parent
Node.stash_child(chid : Node)
Node.unstash_child(chid : Node, at_index : int = -1)
Node.unstash_by_name(chid_name : StringName, at_index : int = -1)
Array<Node> Node.get_stashed_children() const
Node Node.has_stashed_child(chid_name : StringName) const
Node Node.get_stashed_child(chid_name : StringName) const
# On Child
Node.stash_in_parent()
Node.unstash_from_parent()
bool Node.is_stashed() const
Node Node.get_stash_parent() const # get_parent() will not work because node is out of tree.

To make nodes stashed by default, an option will be added to the Scene Tree editor:

image

This will make the node stashed, and the stash icon will appear:

image

Clicking back on the icon will un-stash, similar to other states like unique node.

Then, when instantiating the scene, the node will begin stashed (out of tree). If nothing is done, everything will go on as if the node never was there. Otherwise, it can be brought back by doing

unstash_child( get_stashed_child("DirectionalLight3D") )
# or
unstash_child_by_name("DirectionalLight3D")

If you are using this API from code to disable nodes and back, you can just use the pointers instead:


onready var mynode = $MyNode
...

stash_child(mynode)
unstash_child(mynode)

The lifecycle of these nodes when stashed/unstashed would be the same as when added/removed from the scene, so:

Then on exit:

Live edit integration

Stashing nodes on the editor should use the debugger protocol to send stash/unstash commands to nodes in the running game.

FAQ

Q: Why is it called "stash" and not "disable"?\ A: Because disabling in the context and language of Godot is more related to other functions such as visibility or processing state. The only true way of fully disabling in Godot is removing from the tree, hence this functionality works with that and is called "stashing".

Q: Why not an "active" property in the node?\ A: This is something often requested by users coming from Unity. As mentioned above, Godot has many ways to deactivate parts of a node, by either changing the processing mode or the visibility. Changing those does not completely deactivate a node. In Godot, by design, the only way to make a node 100% and completely inactive is removing it from the tree (and this will not change, for both complex technical an compatibility reasons), hence this proposal is built around pre-existing design constraints to allow solving the same problem in a cleaner and more "Godot" way, where everything will work in a completely predictable way to the user.

Q: I don't care about the editor part that much, is stashing/unstashing the same as completely setting a node to active/disabled?\ A: Yes. Again keep in mind stashing is a bit conceptually different to how it works in Unity because it gets removed from the tree (though still book-kept by the parent, of course). But in practice it can be used the same.

Q: What's the difference with manually adding and removing the tree from the scene?\ A: This is a more elegant way to handle it that also allows to edit the nodes default stashed state visually in the editor, while having them load out of tree. This is not possible (or at least easily) with code currently.

Q: I like this function, but I would prefer the actual stashed are not actually not even loaded if I don't need them. Is this how it works or possible?\ A: The whole concept of stashing is that it remains around until you need it, so these nodes remain loaded and fully configured in memory. Additionally, due to how Godot works, what is in the scene will always be loaded, because resources are loaded before the scene. In some cases, though, you may want the opposite, but for this Godot already has Placeholder function. To do a workflow as you want, you do as follows:

Q: So this is like a dynamic placeholder?\ A: In part yes, but in part this function is meant to be used directly from code too, to make bookkeeping of nodes outside the scene simpler and safer.

Q: Why is not possible to stash by node path, such as $MyNode.stash() and $MyNode.unstash()?\ A: Because when a node is stashed, it is effectively removed from the tree. If it stayed on the tree, external factors such as other nodes, animations, game code, etc. will still interact with it and this would easily result in weird bugs that are hard to figure out, as the node thinks its out of tree. Still, you can cache a pointer and use it to avoid using strings. The proposed API follows the same convention of the rest of the bookkeeping APIs in Node.

Q: Would it not be better to have a stash icon similar to the eye icon for every node instead of an option in the menu?\ A: Despite the demand, It is not clear how much this option will be used. It can begin as an option in the node menu and, only if enough demand, can be moved more prominently (or moved with an option). For the time being, if you need to use this fast, keep in mind the node menu options have shortcuts assigned and you can use that.

If this enhancement will not be used often, can it be worked around with a few lines of script?

N/A

Is there a reason why this should be core and not an add-on in the asset library?

N/A

Norrox commented 1 month ago

Alright, this seems good, and as it follows the core principles of the engine is a plus :D im all for it.

Will this be hard or take long to implement in the engine?

owlnewworlds commented 1 month ago

Great idea and great stuff!

I have only one concern:

In the list of the functions you want to add, there's a unstash_child_at but if I'm correct - since I searched for one already - there aren't any add_child_at function and I've been told it was that way Godot was built so you have to add_child first and then change its position in the tree.

So if that philosophy can't and won't never change, it should be the same for the unstash func: you unstash a node first and then you change its position in the tree.

passivestar commented 1 month ago

The lifecycle of these nodes when stashed/unstashed would be the same as when added/removed from the scene, so:

Enter tree notification and signal. Ready notification (if the first time)

This is great because it will work with people's existing code with no changes required

For the time being, if you need to use this fast, keep in mind the node menu options have shortcuts assigned and you can use that.

As I expect this to be used often by a lot of people (given the interest expressed) I suggest to give it a default shortcut Alt+Shift+S as it's easy to press without moving your left hand. (S is for stash and Alt+S is already taken by the bottom panel)

zydeas commented 1 month ago

From initial impressions, this proposal mostly solves the problems I have a lot more than the original "show process mode beside visibility in the main tree" idea. There might be some finer details that I'm missing right now that would be an issue but overall I think this would meet my requirements if implemented.

I'm not 100% sure about the name "stash". It's technically accurate to what it does, but it's a bit uncommon for what users will actually be using the feature for.

A: Despite the demand, It is not clear how much this option will be used.

I do really think it needs to be on the hierarchy. It's not like it would be completely worthless for me in this state or anything, but it'd be a lot more usable if it was just an exposed button. If you hide it in the submenu then I think a sizable portion of users might not even realize it's there (which includes Unity expats, who'll continue to make posts complaining about it).

A dedicated keyboard shortcut would be nice, but when I'm doing things in the hierarchy window I'll be using the mouse 99% of the time.

Of course, I support the idea of delving further into options to filter which hierarchy icons display that others have mentioned previously so people have the option.

owlnewworlds commented 1 month ago

From initial impressions, this proposal mostly solves the problems I have a lot more than the original "show process mode beside visibility in the main tree" idea. There might be some finer details that I'm missing right now that would be an issue but overall I think this would meet my requirements if implemented.

I'm not 100% sure about the name "stash". It's technically accurate to what it does, but it's a bit uncommon for what users will actually be using the feature for.

A: Despite the demand, It is not clear how much this option will be used.

I do really think it needs to be on the hierarchy. It's not like it would be completely worthless for me in this state or anything, but it'd be a lot more usable if it was just an exposed button. If you hide it in the submenu then I think a sizable portion of users might not even realize it's there (which includes Unity expats, who'll continue to make posts complaining about it).

A dedicated keyboard shortcut would be nice, but when I'm doing things in the hierarchy window I'll be using the mouse 99% of the time.

Of course, I support the idea of delving further into options to filter which hierarchy icons display that others have mentioned previously so people have the option.

Personal opinion: I do like the name "stash" because it doesn't stick the feature to what some (or most) users will do from it. It can have more purposes (maybe) but it will be weird to have something directly named as a reference to one purpose being used for other purposes. Don't know if I'm clear. To summarize: "you need to do this thing, or this thing? You can use the stash feature !"

reduz commented 1 month ago

@zargy

I do really think it needs to be on the hierarchy. It's not like it would be completely worthless for me in this state or anything, but it'd be a lot more usable if it was just an exposed button. If you hide it in the submenu then I think a sizable portion of users might not even realize it's there (which includes Unity expats, who'll continue to make posts complaining about it).

You may be entirely right, but I think in truth there is no clear certainty as whether this will be the case or not, and I don't think its a good philosophy to do something just because that's how Unity does it. Add to this that this feature was not around for 10 years ( despite certainly being useful) and it was not a major productivity or adoption blocker.

I think in our case It's better to be conservative and avoid the extra clutter by default. if eventually turns out we are wrong, we can promote it to a icon just like the eye later on.

yannlemos commented 1 month ago

Amazing contribution. Also, coming from Unity, I believe it scratchs the "enable/disable" itch while still making plenty of sense for Godot.

I'm wondering if it would make sense to slightly decrease the opacity in the name/icon of the stashed nodes. I understand that the icon is there for this, though I feel that for scanning a big hierarchy, differences in opacity could work a little bit better.

Also about @zargy's point on a clickable icon, I think that, due to it being next to other clickable icons which are used very heavily, people are just going to assume it's clickable and if they click it and nothing happens, it might seem like a bug or something a bit arbitrary. I understand that it makes more sense to push it at first without interactivity, though.

All in all, feels pretty good as it is to start with.

passivestar commented 1 month ago

One point needs discussing that wasn't mentioned in the proposal is how it will behave with the EngineDebugger.

For this feature to be of best use for debugging and experiments, when you stash a node in editor while the game is running it needs to be stashed in game. This would allow you for example to disable some enemy half-way through the playtest that you forgot to disable before running the project. Or maybe that enemy was spawned at runtime, then disabling through the editor would be your only way to do it (without additional setup or code). I believe this will require changes to the debugger protocol (which is fine btw because there are a lot of things that it currently can't do that need to be added anyway, some very basic)

Also what will you see in the "Remote" tab when you stash a node through code? Will that tab support the stash icon, or will they just disappear from the tree? If it's possible to implement it in a way that the remote tree still shows them as if they were in the tree with a stash icon it would be useful because it'd allow you to see at a glance what is stashed, and you also may want to be able to unstash through the editor nodes that were stashed through code

reduz commented 1 month ago

@passivestar It should be quite easy to get the editor to reflect the stash state via debugger protocol. Let me add it to the proposal.

MewPurPur commented 1 month ago

https://github.com/godotengine/godot/pull/92377 Also related.

Wolve-3DTech commented 1 month ago

That's exactly what i've waited so long for !

reduz commented 1 month ago

@MewPurPur

https://github.com/godotengine/godot/pull/92377 Also related.

I did not see that one, although I guess this is a superset of that. The nodes in that one are also loaded anyway before being discarded, so it does not change much.

theraot commented 1 month ago

I would expect that animations would need extra care, as there could be animations that use both stashed and not-stashed nodes... And if the behavior is simply that a stashed node acts as if it weren't in the scene tree the animation, the animation would fail because it can't find the stashed nodes.

Janders1800 commented 1 month ago

Stash seems rather unintuitive and I hate the idea of ​​having to go through a pop-up menu to select it. I would prefer to have something like a checkbox, like this (Also not a fan of the name stash, enabled feels better to me);

https://github.com/godotengine/godot/pull/94144#issuecomment-2225974518 348335216-b2d50d32-3910-49da-abba-95842f3b4fa4

eon-s commented 1 month ago

I was thinking of something like this, a Ghost node that works like an InstancePlaceholder but if we can keep the original nodes then it's better.

Can Stashed nodes be added with a more limited processing from the tree? I mean, those nodes "tagged" as stashed by the tree could skip all the lifetime process or at least allow enter-exit the tree but don't run them as ready, keep them always as in "not-ready yet" state.

reduz commented 1 month ago

@theraot

I would expect that animations would need extra care, as there could be animations that use both stashed and not-stashed nodes... And if the behavior is simply that a stashed node acts as if it weren't in the scene tree the animation, the animation would fail because it can't find the stashed nodes.

Yes, but that's the thing. If the stashed node gets removed from the tree then you get an error immediately and you know what broke. If the nodes were somehow disabled but not removed, then the node would not work or do funny things and you would have no idea what is going on.

reduz commented 1 month ago

@Janders1800

Stash seems rather unintuitive and I hate the idea of ​​having to go through a pop-up menu to select it. I would prefer to have something like a checkbox, like this (Also not a fan of the name stash, enabled feels better to me);

Please take time to read the actual proposal text in depth to understand why this is not possible, and please keep feedback constructive.

eon-s commented 1 month ago

Something that may be good as follow up but I think that worth considering on the stashing design, stash nodes but tied with feature tags, so you can have scene branches available in one platform or another and other dynamic situations.

MaxIsJoe commented 1 month ago

Can we use this like an object pool, where we can stash away hundreds of nodes then unstash them when needed? For example, stashing away hundreds of bullet scenes for a bullet hell game. Also, how would this work with features like groups?

reduz commented 1 month ago

@eon-s

Something that may be good as follow up but I think that worth considering on the stashing design, stash nodes but tied with feature tags, so you can have scene branches available in one platform or another and other dynamic situations.

Not unfortunately. The problem is that in scene and resource files, the resources are loaded first and then the nodes. This means that by the time you read this information regarding to stashed status, the resources relevant are all loaded. This cooooooooould be done on export now that the new plugin architecture is working really well, but may be a different feature that needs an entirely different discussion.

@MaxIsJoe

Can we use this like an object pool, where we can stash away hundreds of nodes then unstash them when needed? For example, stashing away hundreds of bullet scenes for a bullet hell game. Also, how would this work with features like groups?

Yes. I have no idea how it would work with groups though.

L4Vo5 commented 1 month ago

Oh, this is basically what I'd proposed in the node_active PR (but obviously a lot more thought out), I do think there's big issues with it though:

To solve these issues, you'd need to:

Another potential solution might be to, instead of using the parent, have some sort of StashedNode type that directly inherits Node and keeps track of a single stashed node. The Node.stash() function:

Setting a node as stashed from the scene will skip that process and directly insert the StashedNode without ever having the node you stashed actually be on the tree.

The StashedNode type then has an unstash() function which reverses this process and deletes itself. Alternatively, calling unstash() on the node that was stashed will call stash_parent.unstash(). This would always preserve the node's relative position in the tree, and it'd allow activating and deactivating nodes easily. The problem is that scene unique names would work, but they could be a reference to the "wrong" node, the stashed one, which could even cause static typing issues. One solution would be to add some functionality to functions like get_node, probably a bool that defaults to true called replace_with_stashed or some better name. If true, and the resulting node is a StashedNode, it will instead return the underlying stashed node.

You might say "but this proposal isn't just for deactivating nodes, there's other use cases for stashing, like node pools", but I don't think anyone was asking for those other use cases to be implemented in this way, so I don't think they should be taken into consideration. (node pools in particular are better if they have the flexibility of letting you put the node anywhere instead of all bullets having the same predefined parent)

reduz commented 1 month ago

@L4Vo5

The problem is as always tradeoffs. The workflow you describe is how placeholders work basically, and the main issue is that this is more cumbersome performance wise (adding and removing nodes is not that cheap and its a hard sell to users that the system to be implemented will be less performant), and usability wise (the node will still be there and your logic, animations, etc. have to check whether its a placeholder or not). It feels to me that if you care about the node ordering, you can just leave another intermediate node and be it.

Added to this you have to remember that this how users already do this in Godot, they stash and unstash manually and this function is far more used than the proposal described here, and the order is not so much an issue in general.

RobProductions commented 1 month ago

I'd have to agree with the concerns of @L4Vo5 , this method seems a little destructive by comparison to the active idea because what will happen to code that references a stashed node? The reference can't disappear because you need to be able to call unstash_in_parent() but if you try to execute a function on a stashed node it errors, right? What happens to signals that were attached to a node that's been stashed? If they are removed for convenience/not throwing an error, do they need to get reattached when a node is unstashed?

There are other complications with workflow that I'm not too sure about. When a node has been stashed, you have to worry about get_stashed_parent vs get_parent, and find_node won't work if you stash something at scene start for quick debugging and rely on getting that reference for later. i.e. this can break existing projects if you start to stash things and never accounted for it before.

In that respect it seems a bit more drastic as it can introduce errors when used for convenience, but I do see the value you gain in being able to stop the existing lifecycle notifications from running. There could still be room for both this and the node active feature as a shortcut to the existing functionality imo because they accomplish the goal in distinct ways that can be preferred/chosen by the developer. If you rely on the hierarchy being intact but still want ease of use for debugging I feel like there's still a lot to be gained with something similar to the original ideas discussed in the other proposal

zydeas commented 1 month ago

@L4Vo5 brings up good points that I hadn't realized. Ideally there'd be some way to restore the specific location of the stashed node under its parent. I don't personally find nodes by their name or a node path that often (I prefer to work with direct references set in the inspector or stuff spawned at runtime), but from what I understand, quite a lot of the community does. So I think it'd be beneficial to consider some way for people to search for all nodes, including ones that've been stashed.

get_stashed_parent vs get_parent is a good point. That should really just be rolled into get_parent (regardless if it isn't actually in the tree). Otherwise, if you need to get the parent of the current node, you'll effectively have to type both of them based on if the node is stashed if because one of them may fail.

@RobProductions I'm not sure I got from the proposal text that manually calling functions on the node would cause any errors. I'd assume that it wouldn't, right?

RobProductions commented 1 month ago

@zargy Was inferring it from this:

If the stashed node gets removed from the tree then you get an error immediately and you know what broke

But you're right, I misread it initially, my bad. When it's removed it throws an error on say animation players that reference it (in which case, how is stashing helpful at all in this situation if you want to debug-disable a child that is part of an animation? You'd just have to go back to visibility and then we're at square one with mixing two different "disable" systems, if this is trying to be that), but could be the case that external scripts can run stuff on it. However, in that case, you end up needing to put extra checks in that code to see if it is stashed, because all the usual functions that work only in tree (get_parent, get_child, etc.) won't work anymore (error? Or just null returns?). So again for debugging, you end up going through extra hurdles to account for stashing in cases like this.

Janders1800 commented 1 month ago

@reduz

Please take time to read the actual proposal text in depth to understand why this is not possible, and please keep feedback constructive.

I'll try to expand on my comment.

I was more focused in the UI/UX part of the proposal, I don't really care how the internals work, but I do care about usability, for a feature that will be used rather frequently, having to do '2 clicks + search' for "stashing" a node I feel like it's too much, It'll get annoying pretty fast in big scenes. There is a reason other engines have easy access to similar features.

And for the name "stash" on the UI side is confusing. IMHO "Enabled" communicates a better intent, internally can be named whatever (but I think everyone prefers consistency here).

From what I've seen in the proposal, I don't see any problem in presenting to the user the feature in the way I posted in the previous comment.

Snowdrama commented 1 month ago

I think this would be a great addition combined with what was already proposed in #7715 and https://github.com/godotengine/godot/pull/94144 While this maybe solves certain things better due to it no longer calling _ready and other tree state callbacks. I don't think it fully replaces the need for a simple toggle. The various things mentioned as cons for a toggle are also pros in some use cases, like it still existing in the tree, being part of groups, not breaking paths and references, still being able to be updated by things like animators. These are all still desirable in some use cases even if disabled. So I don't think it completely removes the need for a toggle for when stashing isn't the ideal behavior.

jonbonazza commented 1 month ago

It would be really nice if a node is stashed, all of its referenced resources are unloaded. This could make the implementation of level streaming incredibly easy.

reduz commented 1 month ago

@jonbonazza that's what placeholder nodes are for, which already work.

reduz commented 1 month ago

@Janders1800

I was more focused in the UI/UX part of the proposal, I don't really care how the internals work

That's the whole point. You can't make a feature that makes you think the engine works in a certain way and then internally when you go to the code it works entirely different. What you expose has to always reflect how it works, specially in a feature as visible as this, otherwise this makes learning and using it inconsistent and confusing.

And for the name "stash" on the UI side is confusing. IMHO "Enabled" communicates a better intent, internally can be named whatever (but I think everyone prefers consistency here).

It' s not better intent if that is not reflecting what it does. Again please take the time to read the proposal.

I think this would be a great addition combined with what was already proposed in https://github.com/godotengine/godot-proposals/issues/7715

Don't disagree, but the question is actually how much this is needed if this one is implemented. That has to be yet determined.

Anutrix commented 1 month ago

Maybe just my opinion but Archive/Unarchive seems name better than Stash/Unstash.

shanayyy commented 1 month ago

Or install and uninstall. 🌝

GuyUnger commented 1 month ago

Even if more technically correct a generic box with "stash" is pretty terrible UX imo. Disabling with a checkbox is much more universally understood and approachable, not only to people coming from unity. A typical example of godot getting more confusing for most people because mostly programming-minded people are making decisions

owlnewworlds commented 1 month ago

Sorry if it sounds stupid since I'm far from being an export in Godot "behind the scenes" and all but if we go along with the "stash" concept and if how to represent it in the editor is a topic in itself, instead of using colors, icons or anything, since the nodes are, indeed, stashed, why not just move these nodes in a new "Stash" sibling node to the Root node? (Well it means there should be another root node lol) Or something equivalent I mean (I'm not an expert of how it works under the hood).

It's something you need to be visible in the editor only at runtime I think. Capture d'écran 2024-07-16 144400

reduz commented 1 month ago

@GuyUnger

Disabling with a checkbox is much more universally understood and approachable

As I mentioned before, the only reason to warrant always visible 1-click access is that this feature is used all the time by most users. this is Usability/UX 101. So, will that happen? I don't really know about that, you don' t either nor anyone else here.

The only thing we do know is that It's been 10 years and community has been pretty much fine for the most part without this, so all of sudden thinking that this feature out of nowhere should get predominance and always-visible status feels like a mistake. It's better to start with a less obtrusive implementation and eventually promote to always visible if enough demand.

CharlExMachina commented 1 month ago

If a node is unstashed, will it keep any signals previously connected, or we'll need to re-connect them again?

owlnewworlds commented 1 month ago

If a node is unstashed, will it keep any signals previously connected, or we'll need to re-connect them again?

I would say it should keep everything connected but ignored ? I don't know if it's even possible.

GuyUnger commented 1 month ago

@reduz Yes I agree adding an icon to every node in the tree without first thoroughly testing its usefulness is maybe not the best idea (my comment wasn't about that). I do think however that it's a very useful feature and calling it something obscure with an icon that intuitively doesn't match it's functionality, is going to prevent more people from using it and adds yet another confusing item to the tree node context menu

passivestar commented 1 month ago

and eventually promote to always visible if enough demand

May I ask what will qualify as enough demand for a settings option for checkboxes?

https://github.com/godotengine/godot-proposals/issues/7715 has 45 upvotes with the discussion heavily focused on the best way to add checkboxes. The PR that aims to close it (which offers checkboxes as an editor option) has 62: https://github.com/godotengine/godot/pull/94144

An alternative PR https://github.com/godotengine/godot/pull/92377 that was opened 2 months earlier by another person which aims to close the same proposal (and again, has checkboxes in the main video) has 51 upvotes

Those are also regular engine users, because this is github, not even a social media poll. As I mentioned earlier, new users who will find the UX confusing won't necessarily come to github to demand things, they'll just decide that godot is not for them 🙂

I'm personally fine with pressing a keyboard shortcut. It works. But will this hurt the adoption for new users because of unconventional UI is still an open question

GuyUnger commented 1 month ago

@passivestar As optional, there might be enough demand to warrant a settings option. But for showing it by default, these proposals you linked in their discussions also have many upvotes for concerns of another icon

passivestar commented 1 month ago

@GuyUnger like I mentioned in one of those proposals, regardless of whether it's shown by default or not I'm all for a dropdown on top of the scene dock to filter them per button, including the default ones, like in blender, because it can already get busy even with the existing icons. If we want to add new functionality to the engine and potentially allow users to add their own buttons via editor plugins it'll need to be solved

GuyUnger commented 1 month ago

@passivestar (as a tangent, FileSystem would be much better with this too, would be so nice if i can choose to have "Open in External Program", "Show in File Manager" and "Copy Path" as buttons next to files)

RobProductions commented 1 month ago

I would say if we are merging https://github.com/godotengine/godot/pull/92377 , https://github.com/godotengine/godot/pull/94144 , and eventually this idea, an editor option for checkboxes specifically is possibly best left for the node active feature as it relates more closely to what a checkbox implies. But I definitely agree with @passivestar in that a blender-like icon filter will be crucial and should definitely be used to create a button that makes stashing more convenient, possibly with that box icon in the OP or some other unique shape.

Another potential downside with this approach: stashing won't work on the root node because there's no parent. Not an issue with your main level scene as you most likely don't care to turn it off, but if you have scenes say for entities that are embedded in your level scene, you can't debug-disable them from their own scene tree. (unless stashing searches through all occurrences of a scene and performs the operation in other scenes, but that's a little convoluted) And similarly for runtime, you'd be forced to stash through code instead of through the editor if you're trying to debug-disable a scene that is spawned after the game starts, which messes with the ease of use sadly...

stuartcarnie commented 1 month ago

Another alternative for names that fit the tree metaphor is prune and graft for a branch of the tree.

reduz commented 1 month ago

@passivestar

May I ask what will qualify as enough demand for a settings option for checkboxes?

Enough demand means how much do users feel the feature is needed after an implementation that is simpler and less visible is done first.

The goal is and always been to keep the user interface as simple as possible because the less new users see they won't often need, the easier the software is to learn. When putting into a scale accessibility and ease of understanding the application on one end and what power users ask for in the other end, the former will always have far more weight than the later.

This does not mean that power users are ignored, but that because precisely, unlike new users they know what they want, they can search for this and find it, or eventually make/download a plugin that does what they want.

So to sum it up, the only reason why something should have top level visibility by default is because it is of utmost importance and frequent access even at early stages of learning only if other alternative solutions have been tried and discarded before.

Additionally, In this case, that many users are asking for something does also not mean they are asking for the same thing. If a pull request or proposal is popular, it very often means (as we established in this case) that users are facing different problems, but root for a common solution that solves them.

In this case, this proposal aims to solve what the majority seemed to express is their problem without going for something that visually explicit and, as you can see this has greatly diminished the amount of demand for other solutions that are more visually explicit.

So, by qualifying as "enough demand" I meant, the demand that will result after this solution is implemented. If the demand is small, then nothing will be done. If the solution implemented does not seem to solve most peoples issues, then something else will have to be done. But the only way to know this is to go step by step.

reduz commented 1 month ago

@passivestar For the record also, I think Blender has a terrible user interface with a lot of duct taped features over it to make it good that had to be added over the years after users complaining for almost two decades about how difficult it was to learn. This is what happens when you aim for productivity over accessibility first.

I'm personally fine with pressing a keyboard shortcut. It works. But will this hurt the adoption for new users because of unconventional UI is still an open question

We never had a problem of that scale in Godot and the adoption has been explosive. This is because our UX philosophy has always been with the focus on learning first, adjusting to power users second (without compromising the first goal). This is entirely on purpose.

We are absolutely still lacking in power user workflows so features like this one and many others are super welcome, but they need to be done in a way they respect the first focus (ease of learning).

passivestar commented 1 month ago

For the record also, I think Blender has a terrible user interface

I never said Blender has a great UI, I actually said the opposite recently. But it has good parts to learn from

We never had a problem of that scale in Godot and the adoption has been explosive

Blender's adoption has been explosive too. But I didn't switch to Blender because of the UI, I switched to Blender despite of it. Because I chose freedom over the luxury of a good UI. But not everyone does, and not everyone will come here to argue about some checkboxes 🙂

I also don't believe that disabling nodes is a power feature. It's like temporarily commenting out code to try different things. When you don't want something in code, you don't open a new text file for it, save it on disk and remove it from your script. You just comment it out. It's basic. It makes sense for it to be discoverable 🙂

I understand your position, you want to do the button in the context menu first and then re-evaluate the demand. I just expect people to still be confused. I guess we'll have to wait and see

Norrox commented 1 month ago

@passivestar For the record also, I think Blender has a terrible user interface with a lot of duct taped features over it to make it good that had to be added over the years after users complaining for almost two decades about how difficult it was to learn. This is what happens when you aim for productivity over accessibility first.

I'm personally fine with pressing a keyboard shortcut. It works. But will this hurt the adoption for new users because of unconventional UI is still an open question

We never had a problem of that scale in Godot and the adoption has been explosive. This is because our UX philosophy has always been with the focus on learning first, adjusting to power users second (without compromising the first goal). This is entirely on purpose.

We are absolutely still lacking in power user workflows so features like this one and many others are super welcome, but they need to be done in a way they respect the first focus (ease of learning).

What about an "advance mode" like gamemaker used to have way back, or "preview mode" where stuff like this apper when enabled? then users can try them out, even beginners. ( EDIT: sorry, just realized this is way off topic ) image

jamesresend commented 4 weeks ago

Although not a perfect, this is much needed. The need to "stash" something that I'm working on while trying to figure out exactly what I want to do with a scene is a much needed functionality. IMO so much needed that when I started with Godot (and take into consideration that this is my first engine to ever try), I was looking for a way to effectively "disable" a node. I would prefer the option for the eye to have 4 stages but right click and "stash" is more than enough for me. But properly document this because people will search for this feature.

passivestar nailed it with this quote:

It's like temporarily commenting out code to try different things.

tokengamedev commented 3 weeks ago

I may be wrong, but this feels like hacky way to handle the node behavior.

Instead of the node having states, it is going to create a group of nodes instantiated but kind of not added to the tree for processing. This way it can handle all the external factors not touching the nodes.

In my opinion, it would have been better the way reduz explained in the https://github.com/godotengine/godot/pull/94144 It should have two states(is_visible, is_processable) and all external factors need to act accordingly otherwise it will have bugs.

As per my understanding, in Godot context active state has less meaning for nodes. It is like enabled state for nodes. It may be applicable to some nodes and not for some. Active state can be applicable for Camera, Popup, windows, Tabs etc., but not for all.

Active state only applies to objects, if there are multiple objects of same type and only one can be active at any one point of time e.g., window or popup objects.

Now coming back to stashing, I have few problems with respect to stashing:

  1. It dilutes the state of nodes. we still do not know what can or cannot be done when process_mode is paused or disabled. May be clear documentation will help.
  2. It further encourages, contributors to ignore the state of the nodes, before acting on it. I am not saying they are intentionally or unintentionally doing it. There may be technical limitations also.
  3. I am still not clear even after reading the whole proposal and comments, what problem does it solves, apart from working around all the inconsistencies in the engine components. That is why I said hacky.

These are my thoughts and opinions.