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

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 opened this issue Mar 22, 2024 · 1 comment

Comments

@Fleischkuechle
Copy link

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
Copy link
Owner

derkork commented Mar 22, 2024

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 (godotengine/godot-proposals#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.

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

No branches or pull requests

2 participants