Implementation of `EngineBitfield` trait #524

Lemiczek · 2023-12-04T17:35:34Z

This PR partially concerns issue #503 and implements decoupling of enums and bitfields in codegen, in a way, where they are split into an already existing EngineEnum trait and the newly implemented EngineBitfield trait, where bitfield ords use u64 whereas enum ords use i32.

Since this is my first PR on the subject of codegen, there may be logical errors in my assumptions, weird code style, code that doesn't need to be there, or general noob-ish code. Please do tell me about any such issues, so I may learn and improve, and subsequently correct them. :)

Since this PR is splitting off enums and bitfields in a more explicit way, this introduces breaking changes for those that have used a bitfield in their projects prior to this change. Specifically on the importing side of things. Not a huge issue, since the language server should pick up on where the type lives and therefore users can easily fix this.

This implementation can lead to potentially more flexibility to bitfields. We have the bitwise operation BitOr. Could BitAnd, BitXor or BitNot be useful to implement in some capacity here?

It is my understanding that methods that use bitfields already use the type, and therefore don't need any refactoring, I may be completely wrong in that assumption. If there are any such instances, we should change them in this PR.

Thanks for the feedback.

GodotRust · 2023-12-04T17:40:35Z

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-524

Bromeon · 2023-12-04T22:44:34Z

Thanks a lot! The code looks already quite good for being the first codegen one 😊
I commented in some places where I see potential for improvement.

This implementation can lead to potentially more flexibility to bitfields. We have the bitwise operation BitOr. Could BitAnd, BitXor or BitNot be useful to implement in some capacity here?

It would be nice, but I'd maybe do it separately and only later (see below for reasoning).

The | operator is mostly to combine multiple bitfield constants to an expression, whereas &, ^ and ! (trait Not, not BitNot) are mostly used for arithmetics, i.e. bit manipulation.

It is my understanding that methods that use bitfields already use the type, and therefore don't need any refactoring, I may be completely wrong in that assumption. If there are any such instances, we should change them in this PR.

~~At the moment, we have the problem that we eventually still need to convert to integers via ord(), because Godot parameters are not annotated with their bitfield types.~~

Edit: It's actually different, but not really better -- Godot exposes some bitfields as enums and their parameters as ints, e.g. ConnectFlags or GroupCallFlags. See #185 (comment) for details.

So there's not that big of a type-safety/conciseness advantage of using bit operators on bitfield types vs. integers.

godot-codegen/src/util.rs

Bromeon · 2023-12-04T22:20:18Z

godot-core/src/obj/traits.rs

+/// Auto-implemented for all engine-provided bitfields.
+pub trait EngineBitfield: Copy {
+    fn try_from_ord(ord: u64) -> Option<Self>;
+
+    /// Ordinal value of the bitfield, as specified in Godot.
+    fn ord(self) -> u64;
+
+    fn from_ord(ord: u64) -> Self {
+        Self::try_from_ord(ord)
+            .unwrap_or_else(|| panic!("ordinal {ord} does not map to any bitfield"))
+    }
+}


The docs should clarify whether an ord represents a single bit flag, or possibly a combination thereof.
I assume it's the former, because otherwise it's not so easy to exclude invalid ords.

Also, the unwrap_or_else error message should be corrected correspondingly; "bitfield" is the type and not the value.

It should definitely represent the single bit flag. 👍

Will change the wording to make sense, sorry about that! :^)

Rephrased the sentences a bit, does this work?

/// Ordinal value of the bit flag, as specified in Godot.` [...] .unwrap_or_else(|| panic!("ordinal {ord} does not map to any valid bit flag"))

godot-codegen/src/util.rs

Bromeon · 2023-12-05T17:07:16Z

godot-codegen/src/util.rs

            }
        }

        impl crate::builtin::meta::FromGodot for #enum_name {
            fn try_from_godot(via: Self::Via) -> std::result::Result<Self, crate::builtin::meta::ConvertError> {
-                <Self as crate::obj::EngineEnum>::try_from_ord(via)
+                #enum_or_bitfield_self::try_from_ord(via)
                    .ok_or_else(|| crate::builtin::meta::FromGodotError::InvalidEnum.into_error(via))


Could you add some docs to FromGodotError::InvalidEnum mentioning very briefly that this also includes bitfields?

Oops, yeah, makes sense.

Although I'm not quite sure on how we should phrase this, other than: /// InvalidEnum is also used by bitfields. 😅

Bromeon · 2023-12-05T17:07:27Z

godot-codegen/src/util.rs

-        .or_else(|| ty.strip_prefix("bitfield::"));
+    if let Some(bitfield) = ty.strip_prefix("bitfield::") {
+        return if let Some((class, enum_)) = bitfield.split_once('.') {
+            // Class-local bitfield


Suggested change

// Class-local bitfield

// Class-local bitfield.

I should probably also change this for the enum also 😅

Bromeon · 2023-12-05T17:07:33Z

godot-codegen/src/util.rs

+                surrounding_class: Some(class.to_string()),
+            }
+        } else {
+            // Global bitfield


Suggested change

// Global bitfield

// Global bitfield.

Bromeon · 2023-12-05T18:56:24Z

Thanks! 🙂

…type" This reverts commit ef8b2b4, reversing changes made to 7dee976.

Bromeon added feature Adds functionality to the library c: engine Godot classes (nodes, resources, ...) labels Dec 4, 2023

Bromeon linked an issue Dec 4, 2023 that may be closed by this pull request

ConnectFlags::ord() and connect_ex().flags() should have a consistent type. #503

Closed

Bromeon reviewed Dec 4, 2023

View reviewed changes

Lemiczek force-pushed the bitflags_u64_type branch from b134043 to 4afc1ea Compare December 5, 2023 16:30

Bromeon reviewed Dec 5, 2023

View reviewed changes

Implementation of EngineBitfield trait

b4cdc29

Lemiczek force-pushed the bitflags_u64_type branch from 4afc1ea to b4cdc29 Compare December 5, 2023 18:05

Bromeon approved these changes Dec 5, 2023

View reviewed changes

Bromeon added this pull request to the merge queue Dec 5, 2023

Merged via the queue into godot-rust:master with commit ef8b2b4 Dec 5, 2023
16 checks passed

TCROC added a commit to Lange-Studios/gdext that referenced this pull request Dec 6, 2023

Revert "Merge pull request godot-rust#524 from Lemiczek/bitflags_u64_…

a188893

…type" This reverts commit ef8b2b4, reversing changes made to 7dee976.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of `EngineBitfield` trait #524

Implementation of `EngineBitfield` trait #524

Lemiczek commented Dec 4, 2023

GodotRust commented Dec 4, 2023

Bromeon commented Dec 4, 2023 •

edited

Loading

Bromeon Dec 4, 2023

Lemiczek Dec 5, 2023 •

edited

Loading

Lemiczek Dec 5, 2023

Bromeon Dec 5, 2023

Lemiczek Dec 5, 2023

Bromeon Dec 5, 2023

Lemiczek Dec 5, 2023

Bromeon Dec 5, 2023

Bromeon commented Dec 5, 2023

Implementation of EngineBitfield trait #524

Implementation of EngineBitfield trait #524

Conversation

Lemiczek commented Dec 4, 2023

GodotRust commented Dec 4, 2023

Bromeon commented Dec 4, 2023 • edited Loading

Bromeon Dec 4, 2023

Choose a reason for hiding this comment

Lemiczek Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

Lemiczek Dec 5, 2023

Choose a reason for hiding this comment

Bromeon Dec 5, 2023

Choose a reason for hiding this comment

Lemiczek Dec 5, 2023

Choose a reason for hiding this comment

Bromeon Dec 5, 2023

Choose a reason for hiding this comment

Lemiczek Dec 5, 2023

Choose a reason for hiding this comment

Bromeon Dec 5, 2023

Choose a reason for hiding this comment

Bromeon commented Dec 5, 2023

Implementation of `EngineBitfield` trait #524

Implementation of `EngineBitfield` trait #524

Bromeon commented Dec 4, 2023 •

edited

Loading

Lemiczek Dec 5, 2023 •

edited

Loading