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

Add support for "inactive" properties #4823

Open
KoBeWi opened this issue Jul 6, 2022 · 4 comments
Open

Add support for "inactive" properties #4823

KoBeWi opened this issue Jul 6, 2022 · 4 comments

Comments

@KoBeWi
Copy link
Member

KoBeWi commented Jul 6, 2022

Describe the project you are working on

Godot etc

Describe the problem or limitation you are having in your project

We have some properties that aren't always useful. Example is Button's expand_icon property. It's useless when you don't have icon. However it can't just be disabled/hidden, because you may want to configure the icon and then add it later.

Generally, there are 3 types of conditional properties:

  • properties that will never be useful if their condition is not met. These can just be hidden. Examples are emission shape sizes in ParticlesMaterial - you only need to see the settings for currently selected shape
  • properties that shouldn't be edited, but seeing their current value is useful. Examples are position and size of Controls inside Container
  • properties that conditionally have no effect, but configuring them might be useful when the dependent value changes. Examples are expand_icon in Button, region_rect in Sprite2D and stretch_margin in TextureProgressBar

The 3rd case is currently not supported properly, i.e. there is a debate whether these properties should be hidden or not, but there is no middle ground, which would be the best in this case.
Some relevant links: godotengine/godot#62423 / #4755

Describe the feature / enhancement and how it helps to overcome the problem or limitation

Aside from hiding or disabling properties, we could have a third option - making them inactive. This would be a bit similar to configuration warnings.
image
When property is inactive, its name would be greyed-out (or have some tint) and the description would have a warning-like message saying why is it inactive.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

Probably add a new property hint PROEPRTY_HINT_INACTIVE, with the optional warning message stored in hint text. This would be alternate option for "disabling" properties in _validate_property().

If this enhancement will not be used often, can it be worked around with a few lines of script?

Nah, no way.

Is there a reason why this should be core and not an add-on in the asset library?

It's core feature.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 11, 2022

Probably add a new property hint PROEPRTY_HINT_INACTIVE, with the optional warning message stored in hint text.

I assume properties would still have their base hint in place, and so the hint string would already be taken a lot of time. And it should really be a usage flag, not a hint flag. Which brings me to...

We already have PROPERTY_USAGE_READ_ONLY. 👀 It was used in the control's layout properties before the rework to prevent editing properties which are controlled by containers (or vice versa, are useless outside of containers). It is still in use in some other nodes.

If your intention is to keep them editable and not completely readonly, then the name inactive is misleading. Maybe adding custom warnings would be enough then. Based on the fact that we have a warning we can additionally decorate the look.

To implement warnings instead of hint text I propose a new method like _get_configuration_warning() but for properties, _get_property_warning(String property_name). It can be executed in a similar manner to _validate_property() to update the warnings. Those warnings would be added just like you've shown, as an addendum in their tooltips.

properties that shouldn't be edited, but seeing their current value is useful. Examples are position and size of Controls inside Container

This won't immediately benefit from the inactive/readonly properties. The problem with this particular PR is that we have a guided layout for properties to prevent common mistakes people make in 3.x. We were asked to show all properties in the discussion preceding this change, but we decided against it. So to bring readonly properties back it would require some thinking on how to keep the layout UI guided while giving more information to power users.

That said, the main issue is probably with the remote inspector, not the local one. And a likely solution is to still show PROPERTY_USAGE_NO_EDITOR props in the remote inspector during the debugging session.


PS. Tangential, but for cases like icon properties we can also try using the indented look with a colored border that I've introduced in the control layouts PR to show that these options are dependent on the icon being present. (They'd need to be rearranged though).

@KoBeWi
Copy link
Member Author

KoBeWi commented Jul 11, 2022

If your intention is to keep them editable and not completely readonly, then the name inactive is misleading.

Not really, "inactive" i.e. "ineffective" - you can set it, but it has no effect.

To implement warnings instead of hint text I propose a new method like _get_configuration_warning() but for properties, _get_property_warning(String property_name). It can be executed in a similar manner to _validate_property() to update the warnings.

Good idea 👀

@Calinou
Copy link
Member

Calinou commented Jul 6, 2023

Can this system be used for properties that require a specific rendering method, but the rendering method in question isn't currently being used? cc @BastiaanOlij

We have an usability issue currently as most properties that require Forward+ and/or Forward Mobile are hidden entirely, but this means you can't disable them after switching to an incompatible rendering method (so you can't silence any potential run-time warnings).

@nlupugla
Copy link

From a UI perspective, I'd prefer the inactive property to be folded away (using essentially the same animation that happens when you open/close a resource in the inspector). This would mean you only see the properties that are relevant to the current configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants