-
-
Notifications
You must be signed in to change notification settings - Fork 97
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
Enhance control over inherited property values #2280
Comments
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. |
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. |
|
I like the idea. I've definitely had several instances where I lost data when using scene inheritance in similar situations.
In my experience this can be a problem both with built-in properties like 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. |
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. |
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. |
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: |
At first, I thought this is a better idea, but inherited scenes would be full of reset icons by default (UI clutter) and derived 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. |
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.
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. |
@RandomShaper |
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. |
Before going more into depth with this PR, I'd like to understand more about:
@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. |
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! |
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...
...if I'd had this instead...
...I'd have avoided most problematic cases. (However, I had a single 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. |
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.
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. |
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'? |
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 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. |
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. |
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). |
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. |
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) |
Yup, basically I had a similar issue Pedro had: #2280 (comment) 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. |
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. |
@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. |
The You can try cloning the |
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. |
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? |
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. |
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, |
This was implemented in godotengine/godot#52943. |
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.
EvilBear.tscn
. I want to create a variant of it,BigHeadedEvilBear.tscn
, inheriting the other scene.Head
inEvilBear.tscn
has a scale of(1, 1)
.Head
's scale inBigHeadedEvilBear.tscn
to(2, 2)
.EvilBear.tscn
, so I set it to(2, 2)
as well.BigHeadedEvilBear.tscn
to do some unrelated editions and save it.EvilBear.tscn
and setHead
s scale to(1.5, 1.5)
and save.BigHeadedEvilBear.tscn
's head is as big asEvilBear.tscn
's. Strange, isnt't it? The last value I set forBigHeadedEvilBear.tscn
'sHead
'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 theHead
'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!
The text was updated successfully, but these errors were encountered: