Allow pinning property values + Consistent property defaults (nicer version)

[RandomShaper]

Implements godotengine/godot-proposals#2280 in a better way than #52233, which is therefore superseded by this one.

This implements what what suggested in this comment to the proposal: godotengine/godot-proposals#2280 (comment) The only thing added on top of that is something that was discusssed below, which is the ability to explicitly pin. (Thanks to @LightningAA for his ideas!)

Consistent property defaults

The first commit in this PR unifies what is considered to be the default value of a property, following the same precedence that the engine uses when instantiating an object. See the comments in the code for details. That means that Godot now has a single, universal idea of what is the default value of a property.

This (which is a follow up of #46270 + #51173) is important for the pinning (and, specifically, auto-pinning) to work reliably.

This first commit also does a lot of cleanup and refactor on the go, to remove duplicate code and make for that single source of truth goal in various aspects.

Allow pinning property values

UPDATE 2: This has changed to make the feature less intrusive for users not needing it. It's now accessible via a context menu. The only exception is if a property is marked as pinned, the pin icon will appear beside the property name. Thanks to @groud for suggesting the context menu approach, which I was able to come up with a good design for.

The second commit is where the actual pinning magic happens.

A key concept here is potential overriding. A property value is potentially overriding if the node has ancestors in the instance/inheritance chain, or if it has a script exporting that property. The latter means that this PR extends the pinnability mechanics to script export values so they also benefit from it. (Conversely, this means that the aforementioned cases where, without the protection brought by pinning, could be unexpectedly changed if some ancestor or the script exporting the variable changes are properly handled.)

Some highlights:

• Pinned and revertable are not separate states. If a property is pinned, it becomes revertable even if reverting it means simply to un-pin it. Pinned and revert are now independent.
• When a scene is loaded, property values that were stored are considered pinned. Force override will be detected when loading a value that is the same as the default. That allows to keep force save and revert independent without having to store a force save flag in scene files.
• Only properties that are stored can be pinned. Two points derive from that:
  • To avoid confusion, a property that is not pinnable won't show pinning UI and its tooltip will explain why will have a disabled context menu item with an explanation about why.
  • Any property that is editor-only, so it's not stored, but is proxies to an editor-hidden real property can still be pinned, thanks to the pin proxy save alias API added to Node (and used in the cases of nodes that needed that). In this PR for master no cases have been found, since those properties are exposed in a different way, but the system is still there because it may be needed. In the version of this PR for 3.x a number of cases have found and been taken care of.

The pinning experience is something users won't almost ever have to worry about and the pinning UI relevant context menu entry only appears where relevant. That also provides the best experience possible to beginners, which won't see anything related to pinning until they start dealing with exported variables or instancing/inheritance.

More images:

---

Version for 3.x submitted as #52944.

[phantomdesvin]

I like the general idea but, in practice, when the user is looking at all those values and see those "restore value" arrow icons, he is not able to know if that icon means that the value is pinned (but equal to its default value) or different to its default value, right? I think that it would be more usefull if it could give that information, if you could tell which values are different than the defaults just looking at their icon.

[RandomShaper]

I like the general idea but, in practice, when the user is looking at all those values and see those "restore value" arrow icons, he is not able to know if that icon means that the value is pinned (but equal to its default value) or different to its default value, right? I think that it would be more usefull if it could give that information, if you could tell if the value is different than the default just looking at the icon.

The fact that pinned and revertable are fused together is actually a beautiful feature of this approach. The revert icon used to mean that the property value will be saved because it's different from the original. With this PR, it just means that it will be saved, and that can be due to the value being different or because it has been pinned. This also avoids the need of having to change the scene file formats to track which properties are pinned. This PR considers that any value explicitly saved is pinned.

One can argue that it may be more flexible to keep both concepts separately controllable, but I believe this way makes a lot of sense, may be easier to beginners and doesn't prevent any workflow.

[phantomdesvin]

One can argue that it may be more flexible to keep both concepts separately controllable, but I believe this way makes a lot of sense, may be easier to beginners and doesn't prevent any workflow.

Thanks for your answer, I understand your point but I can't see it that way. I think that it is not an ideal solution because:

It is not self-explanatory or intuitive for begginers to press an universally recognizable revert icon and that the value doesn't actually change but just stopped being saved instead, with no message or visual information about what is actually happening. The expected behaviour of a revert button is that it will revert the value, not that it will stop it from being saved.

The information that a begginer will gather looking at a list of values with some revert buttons will be that those values has been changed, not that they have been saved.

It prevents or hinders the workflow when a user is looking for properties that have a different value than the original to understand why it behaves differently for example, but the icons don't provide that information, because the icons look like the value has been changed but they could have been only saved.

I'm not an experienced programmer, so I don't know how much of a limitation this could be "This also avoids the need of having to change the scene file formats to track which properties are pinned" but to me this looks like a solution to a programming problem that sacrifices usability so I don't think this is an ideal solution.

[RandomShaper]

The expected behaviour of a revert button is that it will revert the value, not that it will stop it from being saved.

That's the case even without this PR. Reverting means switching back to the original value, which in turn involves not saving the value.

That said, I understand your point and I can see some users may think the same. I guess it's hard to come up with a solution that satisfies everyone. However, the way this is done now is already a fix for a common issue and the way it is implemented opens the door for future refinements.

So, this story doesn't necessarily end here and this can be iterated to add clearer feedback via toast messages and/or a richer, more telling set of icons, and/or something else. Probably this needs to be rolled out so more real world feedback can be collected.

Thanks for your thoughts. You don't need to be an experienced programmer to give valuable feedback, as you have just proven. 😀

[EricEzaM]

Thanks for looking at this issue. I think this is on the right track, but personally I'm not sure if this solution is the most intuitive - or maybe it is just the naming. "Pinning" a value doesn't give much meaning without knowing more context, like the problem it aims to solve. Just throwing it out this idea - let me know if it makes sense - or is even an improvement at all in your eyes.

Basically instead of pinning, you just have two reset buttons. I think this is quite similar to what you already have - with a slight workflow difference.

"Normal Reset". Always visible if it is available. Resets the value to the default - this may come from the parent scene - but the value remains saved in the scene. It will not update when the parent scene changes. Once a value has been changed in a child scene, it will always be considered an 'override' of the parent until a "Hard Reset" is done.

"Hard Reset". Only visible when hovering the property (like your pins). Resets the value to the default and the value is no longer saved to the scene/locally overridden. Updates to the parent will now also affect this child.

Example

1. Parent scene Bear (scale 1, 1) with child scene BigBear
2. Set BigBear scale to (2, 2). The "reset" button appears.
3. Click the reset button on the scale, and it gets reset to (1,1).
4. Change Bear scale to (0.75, 0.75). BigBear will still be at (1,1). The normal reset button resets the value, but does not revert to having the same value as the parent.
5. Press the "Hard Reset" for scale on BigBear, and the value is now (0.75, 0.75).
6. Change Bear scale to (1,1), and now BigBear will also be at (1,1)

I think this is very similar to the "pinning" implementation as I understand it, but to me it makes more sense to have a "revert" and a "hard revert", where revert is just "give me the default value", and hard revert is "give me the default value AND clear my override status, so my value matches my parent again".

So to have an analogy to the current "pinning" system:

• Changing a value away from the default automatically pins it.
• It stays pinned until a hard revert is done.
• Normal revert has the same effect - reverts to the parent/default value but does not clear the fact that it is overriding the default.

[name-here]

• It stays pinned until a hard revert is done.

I like this idea, but I can totally see this leading to an inexperienced user missing the hard reset functionality, using normal reset when it isn't what they wanted, changing the default value, and later being confused when they find the value in an inherited scene hasn't changed. This is a really frustrating interaction, and until they figure it out, they'll think you need to hit reset on the inherited scenes every time you change the default value if you want them to update.

To solve this, "hard reset" and its functionality needs to be really discoverable (basically completely unmissable) and extremely clear about what it does. This is easy to explain through documentation, but easy to get wrong in the interface itself, and some people don't read labels or documentation unless they already know that the thing they want exists.

I'd love to hear what ideas you have for implementing a "hard reset" in the interface (literal Hard Reset button seems a little intimidating), but to me, the one reset button functionality with pinning seems the most foolproof, especially since I usually want to completely reset a property so it tracks the default value.

[RandomShaper]

At this point I would just suggest once more that we iterate once this is in. I mean, what this PR implements is the most basic way of pinning, which doesn't change anything fundamental in how property values were managed so far (well, besides the change about consistent defaults, which could have even been a separate PR and is somewhat transparent to users).

If this is merged, it'd be good to have a bunch of proposals on how to make it more comprehensive or powerful, keeping in mind a balance between those and usability.

[AaronRecord]

If the concern is that it'll be hard to tell when a property is being pinned but not changed from its default value, what if in that case it shows an opaque pin icon instead of a reset icon? Everything else will work the same as it does currently, it'll just be a cosmetic change to the button's icon so you can tell when a property is pinned but not changed from its default value. If you pinned a property and then change its value from the default, then the pin icon will turn into a reset icon, if you changed it back to its default it would become a pin again, etc.

[phantomdesvin]

If the concern is that it'll be hard to tell when a property is being pinned but not changed from its default value, what if in that case it shows an opaque pin icon instead of a reset icon?

The concern is that it is not intuitive, it is not self explanatory.
We need to put ourselves in the shoes of a newcomer. If I see an opaque pin icon, I'm going to think that it is pinnable, not that it is pinned but not changed. The only way that I could ever think the second is if I read that in the documentation. But UI should aim to be self explanatory: If it needs to be explained it has already failed.

[RandomShaper]

Well... Yet another push applyting changes after a new talk with @groud:

• The caption is Pin value.
• The interim commit has been squashed into the most recent since it was becoming too difficult to keep some new changes in sync with both.
• Finally, the pinned flag is again completely independent, but that required storing it as meta.

[RandomShaper]

@akien-mga, this is already OK to both @reduz and @groud (although they haven't approved officially). Just FYI... 😀

This would be a great moment to merge this and its 3.x counterpart for 3.5, now there's plently of time for testing.

Changes done.

[akien-mga]

Thanks!

[reduz]

Ah a bit late, but it looks fine to me