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 template code with Godot 4.4 #1718

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

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Feb 25, 2025

This is just like PR #1715, but instead of synchronizing the variant code, this synchronizes the template code!

The same process was followed:

  1. Delete everything between namespace godot { ... } in godot-cpp's .hpp file
  2. Copy the corresponding code from Godot
  3. Make simple, superficial changes in order to compile. No large-scale changes were made to the Godot code!

I have omitted a couple of things:

  • rid_owner.hpp: Includes some threading stuff that will need some special care
  • spin_lock.hpp: Depends on some OS specific stuff code from within Godot (core/os/thread.h), which we can't just refer to
  • thread_work_pool.hpp: Appears to be loosely based on core/object/worker_thread_pool.h from Godot, which we can already use via the generated code. This maybe this should just be removed?

Similar to PR #1715, it would be nice to get this in before 4.4, so we aren't breaking source compatibility after that release (although, it wouldn't be the end of the world if we did)

@dsnopek dsnopek added the enhancement This is an enhancement on the current functionality label Feb 25, 2025
@dsnopek dsnopek added this to the 4.x milestone Feb 25, 2025
@dsnopek dsnopek requested a review from a team as a code owner February 25, 2025 16:56
@dsnopek dsnopek force-pushed the godot-sync-pre44-templates branch from af78526 to bfc36b4 Compare February 25, 2025 17:02
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.

I'm pretty familiar with about half of this code by now, and I'm not aware of any pitfalls to adapt these files for godot-cpp. It should be relatively isolated.
I guess the only reservation I may have would be to deprecate functions that have been renamed or removed (if any), but I'm not sure if godot-cpp promises to do this anyway between versions.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Feb 25, 2025

I guess the only reservation I may have would be to deprecate functions that have been renamed or removed (if any), but I'm not sure if godot-cpp promises to do this anyway between versions.

This is a somewhat murky area.

We try not to break source compatibility, and (if it's easy) will keep deprecated versions of functions around, even if they are removed in Godot. But we do sometimes break source compatibility (whereas we've only broken binary compatibility the one time, which will also hopefully be the only time ever) and it's "more OK" to break source compatibility at minor version releases, because folks must consciously update - we're somewhat more conservative for patch releases and cherry-pick-able changes.

But, in general, I'm starting to think that maybe we shouldn't do these synchronization PRs for the 4.4 release, and maybe save them for 4.5 so they can get more testing?

I really wish we were already doing this proposal, because then we wouldn't need to worry about timing anything in godot-cpp with Godot's release cycle.

@Ivorforce
Copy link
Contributor

But, in general, I'm starting to think that maybe we shouldn't do these synchronization PRs for the 4.4 release, and maybe save them for 4.5 so they can get more testing?

Hrm, perhaps we want to wait for a patch version or two of 4.4? If Godot releases 4.4, this code should be widely tested on its side, at least, and that should also make it pretty safe to "cherry pick" into godot-cpp.
But even so, I think the risks are fairly small, especially since when in doubt, people can use the 4.3 branch instead which should be fairly stable and still works.

@enetheru
Copy link
Collaborator

enetheru commented Mar 2, 2025

just checked all my compiler /godot variants on windows, and it doesnt run afoul of any issues with the test cases. mind you I am using the build profile, and only running the integration tests.

  • godot-cpp/w64.msvc.windows.x86_64
  • godot-cpp/w64.llvm.windows.x86_64
  • godot-cpp/w64.llvm-mingw.windows.x86_64
  • godot-cpp/w64.mingw64.windows.x86_64
  • godot-cpp/w64.msys2-mingw64.windows.x86_64
  • godot-cpp/w64.msys2-ucrt64.windows.x86_64
  • godot-cpp/w64.msys2-clang64.windows.x86_64

if there are any particular compiler combinations/projects you'd like me to test, I can try to get something running, my local CI is getting easier to manage.

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.

3 participants