Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pinning property values + Consistent property defaults #52233

Closed

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Aug 29, 2021

Implements godotengine/godot-proposals#2280.

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

Peek 2021-09-14 14-46

The second commit is where the actual pinning magic happens. It implements the main idea described in the proposal, but taking some inspiration on one of the approaches suggested there, so the best of both worlds is present.

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:

  • 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.
    • 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 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.
  • Binary scene file format changes so properties can have the new flags field, which so far is onlt used for storing whether the value is pinned. Therefore, the binary scene format compatibility is broken.

  • Text scene file format changes so the pinned flag can be stored, like this: position = Vector2( 20, 0 ) [pinned]. Godot versions prior to this PR will not be able to open scene text files that have pinned values.

  • Some editor settings are introduced:

    • docks/property_editor/auto_pin_on_value_override (defaults to true): If enabled, when you change a property value in the inspector in a potential overriding context the value is automatically pinned.
    • docks/property_editor/auto_unpin_on_value_revert (defaults to true): If enabled, reverting a value (by clicking on the circular reset arrow icon) also unpins it.
    • docks/property_editor/show_pin_ui_only_if_override_possible (defaults to true): If enabled, only properties in a potential overriding context display the pinning UI on hovering.

    WIth all those enabled by default, the pinning experience is something users won't almost ever have to worry about and the pinning UI 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 advanced users are free to tweak the settings for finer control if that's what they want.

More images:
image
image


Version for 3.x submitted as #52234.

@RandomShaper RandomShaper added this to the 4.0 milestone Aug 29, 2021
@RandomShaper RandomShaper changed the title Allow pinning property values in derived/inherited nodes Allow pinning property values + Consistent property defaults Aug 29, 2021
@RandomShaper RandomShaper force-pushed the property_pin_control branch 13 times, most recently from d10cfac to f10090b Compare September 4, 2021 22:46
@fire
Copy link
Member

fire commented Sep 5, 2021

Is this ready for review? I am using the concept of this feature for a property editor similar to godotengine/godot-proposals#1142

@RandomShaper
Copy link
Member Author

Not yet. I hope it will be soon, though.

@RandomShaper RandomShaper force-pushed the property_pin_control branch 8 times, most recently from 3ed565d to bc31d99 Compare September 13, 2021 13:21
@RandomShaper RandomShaper force-pushed the property_pin_control branch 2 times, most recently from 3787628 to d10a1eb Compare September 14, 2021 12:49
@RandomShaper RandomShaper marked this pull request as ready for review September 14, 2021 12:50
@RandomShaper RandomShaper requested review from a team as code owners September 14, 2021 12:50
@RandomShaper RandomShaper force-pushed the property_pin_control branch 5 times, most recently from 0d16b67 to 4b84882 Compare September 17, 2021 12:17
@RandomShaper
Copy link
Member Author

I'll change this back to draft since I'm working on an alternative approach (still compatible with the proposal). The first commit will stay the same.

@RandomShaper RandomShaper marked this pull request as draft September 17, 2021 13:07
@RandomShaper RandomShaper force-pushed the property_pin_control branch 3 times, most recently from a9f2fcf to 047ee87 Compare September 19, 2021 17:32
@RandomShaper
Copy link
Member Author

Superseded by #52943.

@RandomShaper RandomShaper deleted the property_pin_control branch September 22, 2021 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants