Redot-Engine / redot-proposals

Redot Improvement Proposals
MIT License
33 stars 7 forks source link

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

Open Efra-EMX opened 2 weeks ago

Efra-EMX commented 2 weeks ago

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 commented 2 weeks ago

If I understand correct the scope of this proposal is to change the property in CollisionShape2Donly. In that case it should replace all disabledcalls 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 commented 2 weeks ago

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

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 commented 2 weeks ago

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 commented 2 weeks ago

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