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

Rationalize property reversion #46270

Merged
merged 1 commit into from
Jul 31, 2021

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Feb 21, 2021

This PR changes the algorithm used to determine if a reset (circle arrow) icon must be shown on a property in the editor, which means the current value is different from the default one (the one in the scene it instances or inherits from, the default as declared in the script or the default according to the builtin class).

This PR aims to fix some defects in the current algorithm. For instance, the complaint that in a derived scene the value considered the default for a property is the default in the parent script, instead of the one in the parent, which may already be overriding the default in the script, so that is the new default that matters. (See this complaint: godotengine/godot-proposals#2280 (comment))

UPDATE: After an additional push, it also ensures the value a property gets reverted to is the same that was considered to check for "revertability".
UPDATE: Some additional little issues have been taken care of.

Fixes #50781.

UPDATE: Separate version needed for 3.x: #51166

@RandomShaper RandomShaper added bug topic:editor cherrypick:3.x Considered for cherry-picking into a future 3.x release labels Feb 21, 2021
@RandomShaper RandomShaper added this to the 4.0 milestone Feb 21, 2021
@RandomShaper RandomShaper marked this pull request as ready for review February 21, 2021 02:57
@akien-mga akien-mga requested a review from reduz February 24, 2021 10:44
@RandomShaper RandomShaper force-pushed the fix_can_reset branch 4 times, most recently from 37f5fd5 to 36daf9b Compare July 30, 2021 20:00
@RandomShaper RandomShaper changed the title Rationalize determination of property resetability Rationalize property reversion Jul 30, 2021
@RandomShaper
Copy link
Member Author

I'll still do some additional testing and then I'll post a comment when I feel it's ready.

@RandomShaper
Copy link
Member Author

RandomShaper commented Jul 30, 2021

OK. If I tested on some other projects and at least it seems there are no regressions in the most usual use cases.

In any case, it may be better to have in 4.0 for some period of time before considering cherry-picking into 3.x, just in case.

@RandomShaper RandomShaper requested a review from akien-mga July 30, 2021 20:40
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I've run into the issued that this should fix so it's going to make the inherited scene workflow (among other things) much more robust.

@akien-mga akien-mga merged commit 20d46c5 into godotengine:master Jul 31, 2021
@akien-mga
Copy link
Member

Thanks!

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.

Inherited properties always show revert button if the scene it's inherited from isn't set to default
2 participants