Skip to content

Conversation

@bruvzg
Copy link
Member

@bruvzg bruvzg commented Jan 10, 2024

Syncing features with godotengine/godot#86730, but since no GDExtension public API is changed, it should be compatible both ways.

@bruvzg bruvzg added enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation labels Jan 10, 2024
@bruvzg bruvzg added this to the 4.3 milestone Jan 10, 2024
@AThousandShips AThousandShips added the waiting for Godot PRs that can't be merged until an engine PR is merged first label Jan 10, 2024
@AThousandShips AThousandShips removed the waiting for Godot PRs that can't be merged until an engine PR is merged first label Jan 10, 2024
@bruvzg bruvzg marked this pull request as ready for review January 10, 2024 21:41
@bruvzg bruvzg requested a review from a team as a code owner January 10, 2024 21:41
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.

Thanks!

This looks great to me 🚀

I diff'ed the cowdata.hpp and vector.hpp from this PR with the cowdata.h and vector.h from PR godotengine/godot#86730, and they only had minor expected differences. Reading through all the rest, it all looks good.

I haven't tested it manually, but the fact that it's passing the automated tests is a very good sign.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

I know we can merge this before Godot, however, I'm holding off on merging for a bit, just in case there's any further changes to cowdata.h and vector.h on the Godot PR that we'll want to sync over here. When that one seems like it's finished and about to be merged, we can merge here.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

PR godotengine/godot#86730 was just merged. I diffed the vector.h and cowdata.h with godot-cpp's vector.hpp and cowdata.hpp from this PR again, and didn't see any further changes that need to be sync'd. So, I think this one is good to merge now!

@dsnopek dsnopek merged commit 0145e90 into godotengine:master Jan 19, 2024
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

Thanks!

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.2 in #1372

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.1 in #1373

@AThousandShips
Copy link
Member

Assuming this doesn't actually depend on the upstream changes? As they aren't cherry picked, we might want to be careful here as it does have some side effects that might be harmful:

I'd hold off cherry picking this until we have worked things out

@bruvzg
Copy link
Member Author

bruvzg commented Jan 22, 2024

It doesn't depend on upstream change, but won't fix any of the issues without them, so is not useful on its own.

template <class T>
class CharStringT;

SAFE_NUMERIC_TYPE_PUN_GUARANTEES(uint64_t)
Copy link
Member

Choose a reason for hiding this comment

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

This specifically breaks on some platforms, still being investigated

@YuriSizov
Copy link
Contributor

Should fix:

Did it fix this issue btw?

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 24, 2024

Should fix:

Did it fix this issue btw?

Yes it did! I've just closed that issue.

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.2 in PR #1410

@dsnopek
Copy link
Collaborator

dsnopek commented Mar 11, 2024

Cherry-picked for 4.1 in PR #1411

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement This is an enhancement on the current functionality topic:gdextension This relates to the new Godot 4 extension implementation

4 participants