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

Added newer Variant types to typed_array.hpp #1384

Merged

Conversation

allenwp
Copy link
Contributor

@allenwp allenwp commented Feb 6, 2024

This is a companion commit to the godot PR godotengine/godot#87992 which fixes godotengine/godot#87991

IPAddress was added to Godot's typed_array.h recently (April, 2023), so I've added a comment about why IPAddress is not included and how it could be added in the future if this struct is ever added to godot-cpp.

Also undefined typed array templates after use to match Godot's typed_array.h.

@AThousandShips AThousandShips added bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation labels Feb 6, 2024
@AThousandShips AThousandShips added this to the 4.3 milestone Feb 6, 2024
@allenwp allenwp force-pushed the godot-87991-typed-array-additions branch from c57f926 to cd9089c Compare February 6, 2024 17:32
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -116,6 +121,8 @@ MAKE_TYPED_ARRAY(PackedStringArray, Variant::PACKED_STRING_ARRAY)
MAKE_TYPED_ARRAY(PackedVector2Array, Variant::PACKED_VECTOR2_ARRAY)
MAKE_TYPED_ARRAY(PackedVector3Array, Variant::PACKED_VECTOR3_ARRAY)
MAKE_TYPED_ARRAY(PackedColorArray, Variant::PACKED_COLOR_ARRAY)
// If the IPAddress struct is added to godot-cpp, the following could also be added:
// MAKE_TYPED_ARRAY(IPAddress, Variant::STRING)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// MAKE_TYPED_ARRAY(IPAddress, Variant::STRING)
//MAKE_TYPED_ARRAY(IPAddress, Variant::STRING)

No space before code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, thanks!

@allenwp allenwp force-pushed the godot-87991-typed-array-additions branch from cd9089c to 480e397 Compare February 6, 2024 17:59
@allenwp allenwp marked this pull request as ready for review February 6, 2024 18:02
@allenwp allenwp requested a review from a team as a code owner February 6, 2024 18:02
@allenwp
Copy link
Contributor Author

allenwp commented Feb 6, 2024

LGTM

Upon review of recent changes to godot's typed_array.h, I noticed one other change that had become out of sync: the (typed array templates are now undefined after use). I have added this to this PR as a part of syncing this file up with the original.

This is a companion commit to the godot PR godotengine/godot#87992 which fixes godotengine/godot#87991
Also undefines typed array templates after use to match Godot's typed_array.h
@allenwp allenwp force-pushed the godot-87991-typed-array-additions branch from 480e397 to 349b5a3 Compare February 6, 2024 18:53
Copy link
Contributor Author

@allenwp allenwp left a comment

Choose a reason for hiding this comment

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

Added MAKE_TYPED_ARRAY_INFO for new types.

@IvanInventor
Copy link

That's still not all of the possible TypedArray types supported, see this

run/media/vano/hdd/Godot4/godot-cppscript-template/external/godot-cpp/include/godot_cpp/core/type_info.hpp:323:114: error: ‘get_class_static’ is not a member of ‘godot::PackedByteArray’
  323 |                 return make_property_info(Variant::Type::ARRAY, "", PROPERTY_HINT_ARRAY_TYPE, T::get_class_static());
      |                                                                                               ~~~~~~~~~~~~~~~~~~~^~

or this

Basically, whole https://github.com/allenwp/godot-cpp/blob/349b5a3146404ce32c3b6ef8f7427af99fd9dcee/include/godot_cpp/core/type_info.hpp#L354-L388 list should be a copy-paste of whole https://github.com/allenwp/godot-cpp/blob/349b5a3146404ce32c3b6ef8f7427af99fd9dcee/include/godot_cpp/variant/typed_array.hpp#L80C1-L123 list with _INFO suffix, which is a working fix.
I suppose, same thing should be added to engine itself.

@AThousandShips
Copy link
Member

They were already commented out and I think it's for a separate more involved fix as it's possibly not that simple (or it'd have been done already)

@allenwp
Copy link
Contributor Author

allenwp commented Feb 12, 2024

That's still not all of the possible TypedArray types supported

This PR is just for syncing up godot-cpp with the main godot repo, specifically adding the new types like Vector4, Vector4i, and Projection.

All of the other missing types were likely intentionally excluded from godot-cpp because they do exist in the file, but are commented out, as AThousandShips mentioned.

It’s probably best to make a separate issue (and PR) in this repo regarding those missing TypedArray types.

@AThousandShips
Copy link
Member

Indeed, let's keep these separate, the other cases are an old issue and unsure why they are commented out, should be investigated and worked on separately

Copy link
Collaborator

@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.

Looks good to me! Thanks :-)

@dsnopek dsnopek merged commit 7c547c6 into godotengine:master Feb 12, 2024
12 checks passed
@allenwp allenwp deleted the godot-87991-typed-array-additions branch February 12, 2024 16:37
@Innokentiy-Alaytsev
Copy link

Also undefined typed array templates after use to match Godot's typed_array.h.

So how can I declare custom typed arrays now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:gdextension This relates to the new Godot 4 extension implementation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

C++ MAKE_TYPED_ARRAY does not include newer Variant types
5 participants