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

Fix use of non-existent Vector3.Axis enum type in built-in methods and members #99341

Closed
wants to merge 1 commit into from

Conversation

PhairZ
Copy link
Contributor

@PhairZ PhairZ commented Nov 17, 2024

fixes #99309

@PhairZ PhairZ changed the title Fix use of non-existent 'Vector3.Axis' enum type in built-in methods and members Fix use of non-existent Vector3.Axis enum type in built-in methods and members Nov 17, 2024
@AThousandShips AThousandShips added this to the 4.4 milestone Nov 17, 2024
@PhairZ PhairZ force-pushed the that-dont-exist branch 3 times, most recently from 5d8710c to 1f848c0 Compare November 17, 2024 13:02
@PhairZ PhairZ marked this pull request as ready for review November 17, 2024 13:58
@PhairZ PhairZ requested review from a team as code owners November 17, 2024 13:58
Copy link
Contributor

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, this breaks the API in many places and downgrades type safety from enum to int.

Comment on lines +129 to +130
Validate extension JSON: Error: Field 'classes/PhysicsServer3D/methods/generic_6dof_joint_set_param/arguments/1': type changed value in new API, from "enum::Vector3.Axis" to "int".
Validate extension JSON: Error: Field 'classes/PhysicsServer3D/methods/generic_6dof_joint_get_param/arguments/1': type changed value in new API, from "enum::Vector3.Axis" to "int".
Copy link
Contributor

@Bromeon Bromeon Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is the right approach. Even if binary compatibility is retained, this changes the public API exposed towards extensions.

Bindings such as godot-rust and Swift-Godot have been built around a type Vector3Axis (Rust) or Vector3.Axis (Swift). You can't just remove it because of an issue in set_axis(), while it works everywhere else.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I may (as someone who noticed the issue but isn't proficient in C++), the issue seems to be deeper than just set_axis(), since this PR finds and eliminates references to the same enum in multiple classes. I've just checked & found that the enum Vector3.Axis does exist in the code after all, but trying to access it in GDScript results in errors. If eliminating it breaks compat, then perhaps the right approach would be finding a way to expose it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perhaps the right approach would be finding a way to expose it?

I thought the same but no class has exposed enums, so I thought that wasn't the intended approach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precisely my thoughts. These enums from Variants should be exposed appropriately and used across the codebase like Vector3.Axis is. However, doing so is not straightforward because, unlike enums from classes inheriting Object, there's currently no way to do it.

Copy link

@Sithoid Sithoid Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought the same but no class has exposed enums, so I thought that wasn't the intended approach.

Some low-level classes have them; off the top of my head - HTTPClient has enums for Method, Status and ResponseCode.

Copy link
Contributor Author

@PhairZ PhairZ Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Bromeon
I looked at other implementations for other classes like Vector4 and Vector2. None of them had that issue, instead they treated Vector2.Axis params and return types as an int. At least for binded methods. So I thought I'd change any binded method that uses Vector3.Axis as param/return type to int.

I basically matched Vector3's implementation with Vector2, Vector2I, Vector3I, Vector4, Vector4I, and Plane

Copy link
Contributor

@Bromeon Bromeon Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Precisely my thoughts. These enums from Variants should be exposed appropriately and used across the codebase like Vector3.Axis is. However, doing so is not straightforward because, unlike enums from classes inheriting Object, there's currently no way to do it.

There are global enums though. They already contain enums such as EulerOrder specific to geometric builtin types, so Vector3Axis and future Vector2Axis/Vector4Axis wouldn't be that out of place.
DitIt would still technically be a compatibility breakage, but we don't just plain remove a valid feature.

  • There's clear continuity; the binding just needs to unederstand both Vector3.Axis and Vector3Axis in the JSON.
  • There's no downgrade in type safety (int may work for GDScript but is a non-starter for other languages).

Edit: what's not so nice with that is that the constants are effectively duplicated. But retrofitting builtin enums seems like a huge endeavour for these few (others like EulerAngle, VariantType, VariantOperator already exist in global scope and can't just be moved...)

Maybe status quo is still the lesser evil?

Copy link
Contributor Author

@PhairZ PhairZ Nov 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are global enums though. They already contain enums such as EulerOrder specific to geometric builtin types, so Vector3Axis and future Vector2Axis/Vector4Axis wouldn't be that out of place.

It would still technically be a compatibility breakage, but we don't just plain remove a valid feature.

  • There's clear continuity; the binding just needs to understand both Vector3.Axis and Vector3Axis in the JSON.
  • There's no downgrade in type safety (int may work for GDScript but is a non-starter for other languages).

What if we just deprecated VectorX.Axis enums and replaced them with @GlobalScope.VectorAxis that would looks something like this

VectorAxis {
    VECTOR_AXIS_X,
    VECTOR_AXIS_Y,
    VECTOR_AXIS_Z,
    VECTOR_AXIS_W
}

that way its only one enum for all vector types

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about a single enum type; you could then pass VECTOR_AXIS_W to an API that expects Vector2.Axis. If all APIs refer to VectorAxis, the documentation aspect (which dimension) also gets lost.

@Bromeon
Copy link
Contributor

Bromeon commented Nov 19, 2024

Given the discussion above, it seems like this needs some more design.

To sum up the status quo:

  • Vector3.Axis is used in several APIs, but such an enum doesn't exist in GDScript.
  • This PR suggests removing this type and replacing it with int. This is how Vector2.Axis and Vector4.Axis are already handled.
  • The type Vector3.Axis is however referenced in GDExtension JSON and multiple bindings depend on it. There is a clear benefit of strong typing vs. just using int.
  • The current mention of Vector3.Axis is only a documentation issue for GDScript, existing APIs work as expected. Not saying this cannot be improved, but breaking GDExtension compat is (imo) a worse scenario.

Now, moving forward, there are multiple options.

  1. Use int everywhere.

    • While simple, this degrades type safety and usability in my opinion.
  2. Add support for builtin enums, such as Vector2.Axis.

    • Likely a lot of effort and may significantly delay this correction. Not sure if worth it for just the axes.
    • On the other hand, some currently global enums such as EulerOrder or VariantType could become members. But if the old constants have to be retained for compatibility, then this would just add duplication, so the benefit is questionable.
  3. Add dedicated global enums Vector2Axis, Vector3Axis, Vector4Axis.

    • They could have constants such as VECTOR2_AXIS_X.
    • Would work with existing capabilities (global enums).
    • It would mean renaming Vector3.Axis to Vector3Axis -- but since that type needs already special-casing due to not being natively supported, I think that's acceptable.

Personally, I'd prefer option 3, as it would provide a way forward to enhance type safety for other APIs (which currently work with int), while still allowing numbers such as 0, 1, 2 to be passed in GDScript. It doesn't remove a useful feature, and also doesn't require an entire new machinery (builtin enums).

I'd love to hear other opinions on the subject.

@akien-mga
Copy link
Member

Superseded by #99424. Thanks for the contribution!

@akien-mga akien-mga closed this Nov 22, 2024
@akien-mga akien-mga modified the milestones: 4.4, 4.3 Nov 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

set_axis() expects a non-existing Enum Vector3.Axis
6 participants