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

Implement inactive properties #74357

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MarcusElg
Copy link
Contributor

@MarcusElg MarcusElg commented Mar 4, 2023

An attempt to implement godotengine/godot-proposals#4823.

image

Currently an early draft to start the discussion. Imo some solution for dependent properties would improve the usability a lot, as it's currently a bit confusing what properties have an effect, and which don't. Currently it's implemented using a new PROPERTY_HINT that is changed in validate property, should a separate method like suggested in the proposal be used instead?

TODO:

  • Fix errors
  • Support translation Done
  • Use a custom theme colour for invalid properties Done
  • Decide visuals
  • Decide which properties to use this on
  • Expose to gdscript tool scripts?

@MarcusElg MarcusElg requested review from a team as code owners March 4, 2023 12:19
@MarcusElg MarcusElg marked this pull request as draft March 4, 2023 12:19
@MarcusElg MarcusElg force-pushed the inactiveproperties branch 2 times, most recently from fa2ad77 to 1434a6a Compare March 5, 2023 09:43
@MarcusElg
Copy link
Contributor Author

I've implemented a custom theme colour for inactive properties, should it be some sort of gray or some shade of yellow like warnings usually are? Yellow might me a bit too much tho.
image

@MarcusElg
Copy link
Contributor Author

@KoBeWi The issue with using a custom propertyhint as you suggested in the proposal is that it can only have one at the time, so as you see in the image above it looses it's suffix as well as min/max values etc. So maybe it's more logical to have it as a propertyusage instead? In that case there would have be a separate warning value for properties as propertyusages don't have any extra data.

@YuriSizov YuriSizov added this to the 4.x milestone Mar 5, 2023
@MarcusElg MarcusElg force-pushed the inactiveproperties branch 3 times, most recently from 75711a2 to 0274306 Compare March 11, 2023 09:34
@MarcusElg MarcusElg marked this pull request as ready for review March 11, 2023 09:40
@MarcusElg MarcusElg requested review from a team as code owners March 11, 2023 09:40
@MarcusElg
Copy link
Contributor Author

I've changed it to be a propertyusage instead so now it's ready for some feedback

@MarcusElg MarcusElg force-pushed the inactiveproperties branch from 0274306 to 776b072 Compare March 11, 2023 09:42
@MarcusElg MarcusElg requested a review from YuriSizov March 11, 2023 09:42
@KoBeWi
Copy link
Member

KoBeWi commented Mar 11, 2023

Looks good overall, but I'm not convinced about adding another member to PropertyInfo (the property_dependency). I wonder if it couldn't be part of hint_string, like just add it after some semicolon to the existing hint or something.
And if not, PROPERTY_USAGE_INACTIVE is kind of redundant, because you can just check whether property_dependency is not empty.
Need some feedback from @godotengine/core team.

@YuriSizov YuriSizov removed their request for review April 4, 2023 15:41
@MarcusElg MarcusElg force-pushed the inactiveproperties branch from 776b072 to 6829f93 Compare April 7, 2023 09:44
@MarcusElg
Copy link
Contributor Author

@KoBeWi have you discussed anything more regarding this?

@KoBeWi
Copy link
Member

KoBeWi commented Jul 6, 2023

Not really. I forwarded this PR on RocketChat, you can join to discuss it (it would be faster than waiting for a comment in the PR).

@Geometror
Copy link
Member

I think inactive properties should be colored gray, yellow clashes with properties of editable children (see #53260)

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.

4 participants