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

Synchronize most shared variant code with Godot 4.4 #1715

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Feb 21, 2025

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:

  1. Delete everything between namespace godot { ... } in godot-cpp's .hpp or .cpp file
  2. Copy the corresponding code from Godot
  3. Make simple, superficial changes in order to compile. For example, in godot-cpp, we don't use Vector<Vector3> but instead PackedVector3Array, and the EulerOrder 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:

  • The HSL methods on Color: they depend on a third-party library (ok_color.h) which we'll need to figure out how to include with godot-cpp
  • TypedDictionary and TypedArray: these will need special care
  • char_string.hpp: This file differs from Godot's by using more templates, leading to less code duplication. There is PR Core: Integrate CharStringT 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 yet

UPDATE: 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

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Feb 21, 2025
@dsnopek dsnopek added this to the 4.x milestone Feb 21, 2025
@dsnopek dsnopek requested a review from a team as a code owner February 21, 2025 21:07
@dsnopek dsnopek changed the title [DRAFT] Synchronize shared code with Godot 4.4 [DRAFT] Synchronize most shared variant code with Godot 4.4 Feb 24, 2025
@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 24, 2025

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:

  • The HSL methods on Color: they depend on a third-party library (ok_color.h) which we'll need to figure out how to include with godot-cpp
  • TypedDictionary and TypedArray: these will need special care
  • char_string.hpp: This file differs from Godot's by using more templates, leading to less code duplication. There is PR Core: Integrate CharStringT 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 :-)

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!

@dsnopek dsnopek changed the title [DRAFT] Synchronize most shared variant code with Godot 4.4 Synchronize most shared variant code with Godot 4.4 Feb 24, 2025
@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 24, 2025

@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

Copy link
Contributor

@Ivorforce Ivorforce left a 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!

@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 25, 2025

@Ivorforce Thanks for taking a look!

I'm afraid I'm not very familiar with this part of the codebase

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 :-)

@Ivorforce
Copy link
Contributor

I did do a little bit of work on Variant code, but most of the changes seem to be on the math side. I'm less familiar with that!

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform3D::looking_at() missing use_model_front
2 participants