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

Core: Fix built-in enum constant bindings #99424

Merged

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Nov 19, 2024

@dalexeev dalexeev added this to the 4.4 milestone Nov 19, 2024
@dalexeev dalexeev requested a review from a team November 19, 2024 13:25
@dalexeev dalexeev requested review from a team as code owners November 19, 2024 13:25
@dalexeev dalexeev force-pushed the core-fix-builtin-enum-const-binds branch 2 times, most recently from 52a66ff to c202522 Compare November 19, 2024 15:26
@dalexeev
Copy link
Member Author

dalexeev commented Nov 19, 2024

Note: I have not checked the GDExtension and C# binding generators, tests/core/object/test_class_db.h. Also let me know if this change is breaking (I don't know if enum members falling into outer scope is supported in C# and GDExtension).

@dalexeev dalexeev force-pushed the core-fix-builtin-enum-const-binds branch from c202522 to 6206fd5 Compare November 19, 2024 17:34
@Bromeon
Copy link
Contributor

Bromeon commented Nov 20, 2024

Here is the diff of extension_api.json from the previous commit. The AXIS_* and PLANE_* constants are removed as expected. I added some context, so the enums are visible -- which already contain the same identifiers.

Also let me know if this change is breaking (I don't know if enum members falling into outer scope is supported in C# and GDExtension).

A GDExtension binding may technically need an update in the code generator (or an API dependent on it), if constants are missing -- but since the values are still there as enum variants, migration should be straightforward. And since both are passed as numbers to the actual GDExtension C API, binary compatibility shouldn't be broken. Which is the important part, because it means existing plugins will keep working with Godot 4.4 without recompilation.

So from my side this is a welcome improvement! 👍

Copy link
Contributor

@Mickeon Mickeon left a comment

Choose a reason for hiding this comment

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

On the documentation side this is a big plus too

@dsnopek
Copy link
Contributor

dsnopek commented Nov 20, 2024

This shouldn't cause any issues with godot-cpp, since we have hand-written implementations of all the VectorN types and Plane.

Like @Bromeon says above, this could theoretically cause issues with some GDExtension bindings' code generators, but we've made minor extension_api.json changes that could affect code generation in minor releases before (we don't make them in patch releases, though), and I feel like this acceptable and in line with what we've done in the past.

Copy link
Contributor

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Approval for the impact on GDExtension only

@dalexeev dalexeev force-pushed the core-fix-builtin-enum-const-binds branch from 6206fd5 to d89f868 Compare November 21, 2024 16:00
@Mickeon Mickeon requested a review from raulsntos November 21, 2024 19:25
Copy link
Member

@raulsntos raulsntos left a comment

Choose a reason for hiding this comment

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

The C# changes look good to me. I tested it and the generated bindings look as expected.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Looks great to me!

@Repiteo Repiteo merged commit ea3154a into godotengine:master Nov 22, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 22, 2024

Thanks!

@dalexeev dalexeev deleted the core-fix-builtin-enum-const-binds branch November 22, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants