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

Replace "disabled" with "enabled" for consistency #9

Open
Efra-EMX opened this issue Oct 16, 2024 · 4 comments
Open

Replace "disabled" with "enabled" for consistency #9

Efra-EMX opened this issue Oct 16, 2024 · 4 comments

Comments

@Efra-EMX
Copy link

Efra-EMX commented Oct 16, 2024

Describe the project you are working on

Redot Engine in general

Describe the problem or limitation you are having in your project

Currently there are inconsistent property names like enabled, disabled, and even active throughout the engine. All of these properties controls the exact same functionality, so it doesn't make sense for them to be named differently in the first place. For me personally, this looks unprofessional and might be confusing to new user (especially how Disabled ☑️ on looks in the inspector).

This is a minor and highly subjective problem that I'm sure most of us have grown used to (including me), so I'm gathering thoughts if we should keep them as it is, or to "fix" them into one consistent name eventually.
image image
image image
image image
image image

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

The long-term goal would be to convert all of them into one consistent name eventually. I think we can ignore active for now and start from converting disabled to enabled.

While we technically could easily replace all instance of disabled to enabled, we can't afford to boldly do that now when we still want to keep the engine compatible with existing projects and especially upstream (Godot).

With that in mind, I suggest to add enabled property alongside disabled for now. Since disabled is not functionally removed, it should stay compatible. To encourage users to slowly transition to enabled, we can mark disabled as deprecated and also hide it from the inspector.

And perhaps when Redot finally branches off from Godot, we could take more drastic move to replace it for good in the source code (or still let disabled stay deprecated, whichever is preferred by the community).

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

I have applied my solution to CollisionShape2D here as an example: https://github.com/Efra-EMX/redot-engine/tree/disabled-to-enabled

What I did is basically adding a new enabled property, and then hiding disabled from the inspector.
Recording-2024-10-14_15 26 06

Both enabled and disabled are accessible through code, so it should stay compatible with existing projects.
image

I have also updated the docs for both of them, marking disabled as deprecated.
image

This works by adding a new enabled property without modifying anything that involves disabled in the source code. enabled is essentially just a "dummy" property, whose setter and getter simply calls set_disabled() and is_disabled() respectively with inverted value.

void CollisionShape2D::set_enabled(bool p_enabled) {
	set_disabled(!p_enabled);
}

bool CollisionShape2D::is_enabled() const {
	return !is_disabled();
}

To hide disabled from the inspector, I override _get_property_list() and manually erase it from the list.

void CollisionShape2D::_get_property_list(List<PropertyInfo> *p_list) const {
	p_list->erase(PropertyInfo(Variant::BOOL, "disabled"));
}

In all honestly, I am a complete amateur with C++ so I apologize if this is not the best way to implement it.

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

Technically, you can implement the same thing using GDScript:

@export var enabled: bool = true:
    set(value):
        disabled = !value
    get():
        return !disabled

But having to add this script to all nodes that has disabled in every project would be very inconvenient. Not only that, you can't really hide the original disabled from the inspector too. It's simply easier to live with the engine's defaults than to go out of your way to do this.

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

As far as my knowledge goes, add-ons cannot modify the properties of the engine's built-in nodes. I don't think they can hide said properties from the inspector either. Correct me if I'm wrong.

But even if add-ons can do that, as a user, it's easier to live with the engine's defaults than to go out of your way to install add-ons in each project for something so minor. This is a change to what the users see in the engine from the get-go.

@tokengamedev
Copy link

If I understand correct the scope of this proposal is to change the property in CollisionShape2D only.
In that case it should replace all disabled calls within the collision shape class with enabled and disabled should invoke enabled property.
Next disabled property was saved in the scene file. This property should not be displayed but storage should happen in the scene file, for compatibility purpose.

@Efra-EMX
Copy link
Author

Efra-EMX commented Oct 16, 2024

If I understand correct the scope of this proposal is to change the property in CollisionShape2D only.

Not really, I'm talking about the whole engine. The solution I made for CollisionShape2D is more like a "proof of concept" rather than the full change

@tokengamedev
Copy link

Not really, I'm talking about the whole engine. The solution I made for CollisionShape2D is more like a "proof of concept" rather than the full change

This will be big change and will cause conflicts with pulls from Godot engine on various parts of the code. In that case, IMO it should be done as part of major release. as this is not fixing any issue or adding new functionality.

@justinbarrett
Copy link

yes, this is probably a simpler change and one that has bugged me forever. Simple inversion of what already exists would make my mind align a lot more with the UI

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