derkork / godot-statecharts

A state charts extension for Godot 4
MIT License
679 stars 33 forks source link

Small issue with https://github.com/GodotVR/godot-xr-tools/blob/master/addons/godot-xr-tools/objects/grab_points/grab_driver.gd #97

Closed Fleischkuechle closed 3 months ago

Fleischkuechle commented 3 months ago

after starting using your addon i ran into a issue with the xr tools addon i use for my vr development. The problem is in the script: https://github.com/GodotVR/godot-xr-tools/blob/master/addons/godot-xr-tools/objects/grab_points/grab_driver.gd there is a enum that uses a enum with a type State Line 13 (var state : State = State.SNAP) class_name XRToolsGrabDriver extends RemoteTransform3D

Grab state

enum State { LERP, SNAP, }

Drive state

var state : State = State.SNAP

I fixed that by renaming the enum to

Grab state

enum grab_driver_state { LERP, SNAP, }

Drive state

var state : grab_driver_state = grab_driver_state.SNAP

so for future useres maybe you or bastian can either rename the enum or you the class State which collides with the xr tools enum

i already explained the issue on the godot xr discord (https://discord.com/channels/212250894228652034/418572953912082432)

this is meant as a hopefully helpfull feedback.

And thank you for creating such an awesome and really helpfull addon.

derkork commented 3 months ago

Thanks a lot for reporting this. This is similar to #57 just with another class. State is generic enough to be widely used as class/enum name and unfortunately GDScript still has no namespace support (https://github.com/godotengine/godot-proposals/issues/1566).

The upside is, that I can rename this class to say StateChartState with relatively minor impact for existing projects out there as it's an abstract base class and is very likely not used in end-user code and/or scene definitions. It will still be a breaking change but in a local test all the example projects continued to work after the rename with no changes to code or scene necessary. That is something that will not work so nicely for Transition in #57, which will definitely break projects out there but I still think it is worth to do this change for State anyways even if its only a partial fix to the general issue.

Ultimately I really think we need namespaces unless we want to add prefixes to all classes in the hope of avoiding a clash, but even that is not 100% safe.