Enhance control over inherited property values

RandomShaper commented 3 years ago

Describe the project you are working on

2D action platformer game (but I guess this will apply to many other kinds of projects).

Describe the problem or limitation you are having in your project

In short, on some cases it's hard to control how property values propagate to inherited scenes.

Say I have some enemy as a scene, EvilBear.tscn. I want to create a variant of it, BigHeadedEvilBear.tscn, inheriting the other scene.
The node Head in EvilBear.tscn has a scale of (1, 1).
I set the Head's scale in BigHeadedEvilBear.tscn to (2, 2).
I have both scenes saved by now.
Later, I decide I the big head looks good for the "normal" EvilBear.tscn, so I set it to (2, 2) as well.
I save the scene and switch to BigHeadedEvilBear.tscn to do some unrelated editions and save it.
I change my mind again: the "normal" bear should have a smaller head, so I go to EvilBear.tscn and set Heads scale to (1.5, 1.5) and save.
I run the game and see that BigHeadedEvilBear.tscn's head is as big as EvilBear.tscn's. Strange, isnt't it? The last value I set for BigHeadedEvilBear.tscn's Head's scale was (2, 2), not (1.5, 1.5).

What happened is, of course, that the editor, upon saving BigHeadedEvilBear.tscn on step 6 realized that the Head's scale was the same as in the parent scene, so it assumed there was not longer a need to explicitly store the value. But I wanted to keep it and, in general, avoid those cases of unintendedly losing manually set values when they happen to match the new inherited ones.

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

My idea is to add a way to disambiguate (thanks Wikipedia for such a cool word), on those cases where a property value matches the inherited one, the intent between "I want whatever comes from the parent scene" and "I want to keep this value I set no matter what happens in the parent scene."

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

Let's consider again the case where the parent and the child scenes have the same scale values for some node:

I'm at the child scene and want to "pin" that value so it's there regardless what happens in the parent scene. Therefore, I hover the area of that property value in the inspector and this appears (only if hovering, to avoid cluttering the inspector):

I can then click on the pin icon so it stays (that means it's visible in the inspector without need to hover, exactly like you see the reset icon when appropriate):

That will make the editor save that property value regardless it matches or not the one from the parent.

This is compatible with the reset icon, as shown here:

If you click reset, both icons disappear (well, the pin icon is shown in low-intensity because you are still hovering that area, until the mouse pointer goes away) and you have the chance again to pin the "resetted" value if that's what you want.

This is also useful for default values in scripts. In other words, this would be the universal way to stop the potential tampering from values coming from higher levels.

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

I don't think so. One could say that these cases could be overcome by more carefully design of the scene hierarchy, but, let's be honest, many times you'll find yourself in a trap like this when the project is in an advanced stage of development and it's too late to go back and re-plan. Also, given that a big strength of Godot is how flexible it is when it comes to designing your scenes workflow, this addition would just make it even more powerful.

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

There's no way to implement this outside the very guts of Godot scene system!

reduz commented 3 years ago

Would not an export hint to always save the property be better? I heard users complain about this, but most of the time its related to the custom properties they export.

RandomShaper commented 3 years ago

An export hint would work only for custom properties defined in the script, not for builtin. (Well, to be honest, I can imagine some syntax in the script to specify always save for builtin properties, but that takes us to the next point.)
It would be a cumbersome to have to make proxy properties just to support that on builtin properties. You'd be cluttering the inspector with two similarly-looking properties and also cluttering your scripts.
And, if you take that route, you are settling how that property works for every scene with that script attached, losing the ability to switch between pin and inherit.

That said, if most users complaints are about exported variables, the export hint idea may fit better. I have no data. I can only say that I've been bitten by this exclusively with builtin properties.

KoBeWi commented 3 years ago

~~This looks like the pins might clutter the inspector. There should be some button that toggles them (but enabled pins could be always visible).~~ Nevermind, I totally didn't see that you already covered it.

RandomShaper commented 3 years ago

This looks like the pins might clutter the inspector. There should be some button that toggles them (but enabled pins could be always visible).

That's the idea. In the unpinned properties, you only get to see the pin icon while you are hovering.

No hover:

Hover:

akien-mga commented 3 years ago

I like the idea. I've definitely had several instances where I lost data when using scene inheritance in similar situations.

Would not an export hint to always save the property be better? I heard users complain about this, but most of the time its related to the custom properties they export.

In my experience this can be a problem both with built-in properties like scale or texture and indeed with custom properties with game design parameters (speed, strength, etc.).

So I think we can mix both suggestions: implement a property usage hint to enable always saving the property, and expose it in the Inspector via a pin like suggested.

This would also make it possible to pin project settings which do not get saved due to their default value, but where this can be an issue in upgrades (renderer, physics engine). For example in 3.2 the default renderer is GLES3, so this doesn't get saved if you use GLES3. When upgrading to Godot 4.0, the project would now use Vulkan which is the new default value, even if there was a non-default GLES3 option.

NathanLovato commented 3 years ago

I'm totally for the changes suggested in this discussion and proposal. We stopped using and teaching inherited scenes due to this limitation: people feel like they're losing saved work, and the current way inherited scenes are, it's easy to forget you have to be extra careful, even as a professional.

We've introduced bugs in our demos several times out of modifying and saving a parent scene because it cleared overridden properties on the inherited one. So we stopped using it altogether.

Sebjugate commented 3 years ago

Wouldn't it be better to simply not remove the overridden value if the child scene happens to match the parent? This seems to be what people expect as a default anyway. Then, if you change your mind and do want the child to inherit the value from the parent, you use the existing reset functionality.

2plus2makes5 commented 3 years ago

I see parent's values as default values, so children's values should match parent's ones only if they are using "default" values, or said in other terms, values should match parent's values only if they aren't overriden, that would be the simplest and cleanest solution in my opinion.

EDIT: if i can give a suggestion don't use parents in game, fill them with just the stuff that all its children have in common and use only the children in game(in your case you could make basic_bear as parent and then small_head_bear and big_head_bear as chlldren), it doesn't solve the issue, but doing this way limits these cases.

RandomShaper commented 3 years ago

Wouldn't it be better to simply not remove the overridden value if the child scene happens to match the parent? This seems to be what people expect as a default anyway. Then, if you change your mind and do want the child to inherit the value from the parent, you use the existing reset functionality.

At first, I thought this is a better idea, but inherited scenes would be full of reset icons by default (UI clutter) and derived .tscn files would need to store all the values by default, even if they are the same as in the parent.

But, more importantly, you would lose the ability to tell the editor, "I want to reset this property to the value in the parent scene and also have it saved in the derived one". You'd have to go back to the parent scene to see the parent value and then set the child value to it, manually, carefully avoiding to hit reset, because by clicking reset you'd be telling the editor to stop overriding, with no way back.

We'd need to add a new icon per property to switch back to overriding, so your idea would become the override-by-default version of this proposal and I believe Godot should stay inherit-by-default.

RandomShaper commented 3 years ago

I see parent's values as default values, so children's values should match parent's ones only if they are using "default" values, or said in other terms, values should match parent's values only if they aren't overriden, that would be the simplest and cleanest solution in my opinion.

How would that affect the proposed solution? The problem now is that once the value matches between parent and child it's no longer clear if the intent is to keep overriding (so new values from the parent are not propagated even if they change in the future) or to start inheriting unconditionally.

if i can give a suggestion don't use parents in game, fill them with just the stuff that all its children have in common and use only the children in game(in your case you could make basic_bear as parent and then small_head_bear and big_head_bear as chlldren), it doesn't solve the issue, but doing this way limits these cases.

That indeed seems like a good way to avoid the problem in the first place. A good thing of this proposal is that it won't stop you from doing that or need you to do anything special to use such a workflow, but should you happen to be trapped in that situation, you have a way out.

2plus2makes5 commented 3 years ago

@RandomShaper Well yes you are right, during development people changes values a lot so they shouldn't be worried about other classes values, i guess i'm too used being overly cautious and to the way i'm doing things that i didn't see the clearly how the issue affects others.

AndreaCatania commented 3 years ago

I'm for it too. I keep lose my data too, so with this I would be 100% sure that such data doesn't change. Honestly, it's even well integrated with the editor.

reduz commented 3 years ago

Before going more into depth with this PR, I'd like to understand more about:

How much of this is a problem for scripts only, and how much is it for general scene properties.
How much of the data losses are expected due to the design, or they are bugs that can be fixed.

@RandomShaper @NathanLovato @AndreaCatania could you please share a bit more of your background on these topics?

I remember that @kubecz3k also had issues with this, so would appreciate feedback from him too, so tagging him.

NathanLovato commented 3 years ago

For me, it's an issue for everything you override in the Inspector in the inherited scene and that can get lost.

The ideal use case is that you can be sure you don't lose any values and overrides in the child scene. Be it an overridden value in the inspector or a variable you rename, more on that below.

At least, if that's not possible, getting some kind of optional warning when reparenting nodes or changing an exported variable's name would be better than silently losing data, which currently happens.

Some context.

When coding, I first solve problems the simplest and fastest way possible, then refactor bits to keep the code readable.

When I use inheritance between script files and refactor or change something's behavior, I get errors for everything I broke in the extended class. If I renamed a variable and didn't update it on the extended class, I'll also get an error and a chance to rename it and preserve the override.

That's the kind of experience I'd like and expect from inherited scenes, even though I understand the technical challenge.

There's one limitation of exported variables that's related: when changing a variable name in a script, the scene's value gets lost, and so it propagates to inherited scenes, making refactoring difficult.

Let me know if you have any questions!

RandomShaper commented 3 years ago

Most of my practical issues (where I actually lost values I wanted to keep) have been with builtin properties, but the bad lesson learnt created for me also the burden of checking if values in exported script properties were still good after doing some changes (never turned out to be the case).

Some cases would have been avoided by using a different scene design; for instance, where I have something like...

RatA
+-- RatB

...if I'd had this instead...

BaseRat
+-- RatA
+-- RatB

...I'd have avoided most problematic cases.

(However, I had a single Rat and the idea to let it be RatA and also the base for RatB happened later in development and I found it easier to do it that way.)

For the cases where I didn't actually lose values but could have happened, the scene design was already like the one on the second inheritance tree.

Sebjugate commented 3 years ago

inherited scenes would be full of reset icons by default (UI clutter) and derived .tscn files would need to store all the values by default, even if they are the same as in the parent.

I was thinking the behavior would be the same as it currently is (only store values that are overridden). Simply remove the process that automatically erases the stored values if the parent becomes the same as the child. It would be the same amount of storage and reset icons as currently, just without the "automatic cleanup" if a parent is changed.

I didn't even realize that Godot "cleaned up" those overridden properties and that seems unintuitive to me as a whole. I'm surprised I haven't encountered it myself yet, to be honest. If I've overridden something, I can't think of any time I wouldn't want it to stay overridden (that is, I can't think of any time I'd want the current behavior). Adding the pin functionality just seems like adding more complexity rather than addressing the underlying unintuitive behavior. Although I assume the current behavior was implemented on purpose, and someone had reasons for it.

But, more importantly, you would lose the ability to tell the editor, "I want to reset this property to the value in the parent scene and also have it saved in the derived one". You'd have to go back to the parent scene to see the parent value and then set the child value to it, manually, carefully avoiding to hit reset, because by clicking reset you'd be telling the editor to stop overriding, with no way back.

That is true. It would be handy to have that ability. Although I suspect this would be the rarer case. At least for me. It seems unlikely that I'd know ahead of time I'm going to be changing the parent's default. Otherwise I would have just set it? I could be missing something.

RandomShaper commented 3 years ago

I was thinking the behavior would be the same as it currently is (only store values that are overridden). Simply remove the process that automatically erases the stored values if the parent becomes the same as the child. It would be the same amount of storage and reset icons as currently, just without the "automatic cleanup" if a parent is changed.

Maybe I misunderstood. I'm trying to picture in my head how that would work. For that, please clarify this (and correct me if I'm making some wrong assumptions):

You create an inherited scene. It defaults to inherit values from the parent scene. No reset icons are displayed. What shall the user do to mark some property as 'keep this value'?

creikey commented 3 years ago

I have had some data loss occur due to a script property rename. Imo there should be an "orphan properties" section on the inspector full of property values you've set before but no longer exist on the node+script. As well as this, a new decorator could be added to exported variables such as legacy("old_name") then when loaded and ran the orphaned legacy value will be used, and when opened in the editor Godot will automatically move the legacy parameter names to the new one and resave the file.

All facets of how scenes work should be redesigned to reflect a "user data is of the utmost importance" philosophy, where no information is ever lost without manual intervention or at least some kind of notification.

AaronRecord commented 3 years ago

You create an inherited scene. It defaults to inherit values from the parent scene. No reset icons are displayed. What shall the user do to mark some property as 'keep this value'?

If the user changes the value in the inherited scene.

RandomShaper commented 3 years ago

If the user changes the value in the inherited scene.

So I guess that if what the user wants is to force the current value to stay regardless what happens to the parent scene, they would have to start editing the value and then confirm without changing it, right? That would make the reset icon appear so you know you are overriding even if it's the same value.

I can see some elegance in leveraging the current UI elements, but the way of "pinning" values is not very user-friendly. Users would need to known how to re-enter the current value for many types of property editors, including colors, dropdowns, etc. That's where I see the pin icon is better, by being one-click and property type agnostic.

AaronRecord commented 3 years ago

Basically, if you change a value in an inherited scene, it should automatically get "pinned", and if you hit the reset button it should get "unpinned", and there's no reason to show a pin icon.

The only use case I see for "pinning" properties but not changing them from the parent's value is if you're already planning on changing parent's property but you haven't gotten around to it yet. If that's a real problem, then I would suggest if you hover over a property in an inherited scene that's not "pinned", then a translucent reset (or pin) icon should appear, and if you click on it, the property's value won't change, but the reset icon will turn opaque (or replace the pin icon) (the property is now "pinned", and you can hit the reset icon again and it'll "unpin" the property).

RandomShaper commented 3 years ago

That doesn't sound bad at all. At this point, the only thing I don't quite like in your idea is the unreset/pin UX. Neither options (translucent reset icon, or also translucent pin icon that becomes reset icon) sound like easy to understand, but maybe it's a matter of mocking up to really see how they feel. If we could iron that out, this approach could be the holy grail.

AaronRecord commented 3 years ago

Will/should this also apply to instances of scenes that override their default values? I think it'd make a lot of sense if it did.

Also, when messing around I also found some more weird/unintuitive behavior, the reset button will appear on properties in inherited scenes even if they match their parent's, if the parent's value overrides the script defined default (hitting the reset button does nothing though).

(B inherits A)

AndreaCatania commented 3 years ago

Yup, basically I had a similar issue Pedro had: https://github.com/godotengine/godot-proposals/issues/2280#issuecomment-778335475

I had a scene A inherited by the scene B, where the scene B was already fine and I had to change the scene A. Changing the scene A also changed the scene B so I had to re-tweak the scene B.

RandomShaper commented 3 years ago

Will/should this also apply to instances of scenes that override their default values? I think it'd make a lot of sense if it did.

I think it would. However, if the "natural" way to implement what is addressed here somehow excludes that, we'd have to consider it a separate issue to be fixed anyway. I also find it confusing.

RandomShaper commented 3 years ago

@LightningAA, regarding the misleading reset button in scripts, you may want to check (and maybe even test) this one: https://github.com/godotengine/godot/pull/46270

AaronRecord commented 3 years ago

@LightningAA, regarding the misleading reset button in scripts, you may want to check (and maybe even test) this one: godotengine/godot#46270

Do I need to clone the PR and build it or are their auto generated builds of it (sorry, I'm new to this, I saw that it checks if it'll build for different platforms but it didn't look like it saves them)? Also I'm not sure I can test yet because godot 4 won't run on my laptop because it doesn't support vulkan (intel hd 4400, "Your video card driver does not support any of the supported Vulkan versions"), is there a way to make it use OpenGL instead? Regardless, it sounds promising.

Calinou commented 3 years ago

is there a way to make it use OpenGL instead?

The master branch doesn't have an OpenGL renderer yet (and won't have one for a few more months at least), so I'm afraid not.

You can try cloning the 3.2 branch of this repository and applying the commit on top, but there may be merge conflicts you'll have to resolve before you can compile. You can do this by saving https://patch-diff.githubusercontent.com/raw/godotengine/godot/pull/46270.patch in Godot the repository root then running git am 46270.patch in a terminal at the repository root.

FeatherAntennae commented 3 years ago

For the UI / UX, I think a good example on how to tackle this would be how Blend / Visual Studio work with xaml properties. They use a color coded square to designate the type of value or the type of binding giving the value of a property.

In our case, it doesn't have to be that complex, but I think it's a good approach.

(The popup menu appears when you click on the coloured square) Blend_qnyM2wjQDY

Edit This could also be reused for a bunch of things, like indicating if the property is bound to an animation, if it is set by script, if it's the default value, a reset button, etc.

mpenney99 commented 3 years ago

Wouldn't it be better to simply not remove the overridden value if the child scene happens to match the parent? This seems to be what people expect as a default anyway. Then, if you change your mind and do want the child to inherit the value from the parent, you use the existing reset functionality.

Looks like this comment got somewhat buried. This approach seems very logical to me. The existing behavior is very unexpected, so I would suggest to change that rather than implementing new functionality which might be easy to miss.

RandomShaper commented 3 years ago

How would that work? You have scene A and create a scene B that inherits A. Should all the property values in B that at that point match all the ones in A be considered overrides? Such logic would cause every child scene to include all the property values from the parent by default (child scenes would contain even more stored values than parents!). That's not normally what you want. Normally, you let values from parents flow into children.

What would be the logic to start and stop considering a value overridden so that doesn't happen?

mpenney99 commented 3 years ago

By default, all properties are inherited from their parent, but when a child property is changed in the inspector, the value becomes fixed and the refresh icon is displayed. This is basically the current behaviour.

However, the problem occurs when the parent property is changed to the same value as the child. For some reason, we remove the child override in that case. This is weird - the child property should continue to override, even it's overriding with the same value.

RandomShaper commented 3 years ago

I see. That's a valid way of seeing it, indeed. However, in the current implementation I've favored a more explicit control over that to avoid that the inspector shows anything related to force override by default, to avoid confusion to beginners. Given this is not needed very often, I can't but see that gives the better balance.

That said, as I usually say, if when this is released real world usage tells otherwise, we can always iterate and change these mechanics,

RandomShaper commented 2 years ago

This was implemented in https://github.com/godotengine/godot/pull/52943.

godotengine / godot-proposals