-
-
Notifications
You must be signed in to change notification settings - Fork 615
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
Synchronize most shared variant code with Godot 4.4 #1715
base: master
Are you sure you want to change the base?
Conversation
bc20252
to
dac7836
Compare
dac7836
to
6637edb
Compare
I've decided to limit this PR to the variant code (not all shared code) and to omit a couple of things, in order to reduce the scope of this PR, and get it out of DRAFT sooner. Things I have omitted:
With those caveats, I'm going to take this out of DRAFT. If folks think we should try to address more within this PR, rather than splitting those things out to their own PRs, please let me know. But this is already a lot to review! |
@Repiteo @Ivorforce Can you folks review this PR when you have a chance? You were involved in some of the changes that are being ported over from Godot and are both quite familiar with Godot core |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, what a big changelog. Almost makes me wish Godot had a monorepo so we could auto-sync with at least the templating headers 😅 Thank you for going through the effort of syncing!
It looks like this PR is mostly math changes, along with quite a lot of style changes. I'm afraid I'm not very familiar with this part of the codebase, but I think it's fairly isolated and unlikely to cause issues when we update.
Overall, I'm not aware of any pitfalls to sync the changes, and I didn't notice any either when skimming through it. The changes you list made to the code seem sensible to me. We should probably test this, but other than that, it looks good to me!
@Ivorforce Thanks for taking a look!
Ah, I thought you were involved in the variant code in core. Maybe just the template code? I've also just created PR #1718 to address the shared template code :-) |
I did do a little bit of work on |
There is a bunch of code that is shared with Godot that we regularly synchronize by copying the Godot version and modifying to work in godot-cpp.
We haven't done that in a while!
The goal of this PR is to completely synchronize most of the shared variant code, ideally, in advance of the Godot 4.4 release. (Although, there is a lot of stuff to work through!)
The process is basically:
namespace godot { ... }
in godot-cpp's.hpp
or.cpp
fileVector<Vector3>
but insteadPackedVector3Array
, and theEulerOrder
enum has different values in godot-cpp, etc. No large-scale changes were made to the Godot code!I have omitted a couple of things:
Color
: they depend on a third-party library (ok_color.h
) which we'll need to figure out how to include with godot-cppTypedDictionary
andTypedArray
: these will need special carechar_string.hpp
: This file differs from Godot's by using more templates, leading to less code duplication. There is PR Core: IntegrateCharStringT
godot#98439 to port these changes into Godot, so rather than revert to the Godot version, we'll wait for Godot to update to this version :-)Marking as a DRAFT because I haven't gotten through everything yetUPDATE: I've decided to limit this to the variant code (not all shared code) and to omit a couple of things, in order to reduce the scope of this PR, and get it out of DRAFT sooner.
Fixes #1644