Skip to content

Conversation

@zorbathut
Copy link

Works, somewhat. Various notes:


This set of changes makes it so you can set the boot image depending on screen ID, which requires adding a parameter to RenderingServer::set_boot_image. However, RenderingServer::set_boot_image is part of the GDExtension API, which appears to mean it must not be changed. The new boot-image behavior seems potentially useful, but not critical, and I'm not sure how extensions like this to a deployed GDExtension API are meant to be deployed. I'm pretty sure this is not necessary for proper functioning, either in libgodot mode or conventional mode, and there was at least one place where I think this caused a bug (the RenderingServer::get_singleton()->set_boot_image() in main.cpp:3590 would implicitly convert bool boot_logo_filter into a 0-or-1 integer screen ID), so I've reverted this change. If you think this is critical and if someone can explain how stuff like this is meant to happen with GDExtension, I'm happy to reintroduce it (without the bug :V)


MethodInfo::hash and the "hash" variable inside MethodInfo::get_compatibility_hash conflicted. I chose to rename the variable inside MethodInfo::get_compatibility_hash. This feels slightly warty because now MethodInfo has two different things named "hash", one calculated by MethodBind, one calculated by MethodInfo. This is extra-weird because MethodBind::get_hash works by creating a MethodInfo and calling get_compatibility_hash(), and this is what is stored inside MethodInfo; also, one of the callers of MethodInfo::get_hash is info_from_bind, which also creates the exact same MethodInfo while calling the function that reproduces some work.

So in summary, in info_from_bind, we create a MethodInfo, and we want to fill out its members; in order to fill the hash member, we call a function on MethodBind, which creates an entirely separate MethodInfo, fills that out, and calls get_compatibility_hash() on it, then throws that half-filled-out MethodInfo away so we can finish filling out the MethodInfo that previously existed.

This could probably be made more efficient.

I also don't care enough to do this for this specific change, it's probably fine, it looks like to the extent it's an unnecessary slowdown it's just a slowdown in ClassDB::get_method_* and my gut feeling is that if this is a bottleneck for anything, a better solution is "stop calling it so often".


The #undefs are incredibly ugly. The fundamental problem is that Win32 headers redefine MemoryBarrier. We do have a file that's meant to undefine it (typedefs.h), but it is itself include-guarded and doesn't include all the windows headers, which means it's a one-time fix for an issue that can re-emerge repeatedly as later files include different Windows headers. Worse, this can re-emerge unpredictably; one bug I ran into was:

  • native_menu_windows.cpp includes various things that include "typedefs.h", consuming the typedefs.h include guard
  • native_menu_windows.cpp includes "native_menu_windows.h"
  • native_menu_windows.h includes <windows.h>, introducing the MemoryBarrier redefine
  • native_menu_windows.cpp includes "display_server_windows.h"
  • display_server_windows.h includes "joypad_windows.h"
  • joypad_windows.h includes "os_windows.h"
  • os_windows.h includes "rendering_device.h"
  • rendering_device.h gets parsed for the first time, corrupting internal struct names thanks to the MemoryBarrier redefine
  • this doesn't get noticed unless something also tries to use those internal structs . . . but once someone does, it's really hard to track down.

There's just no obvious single place to put this fix; typedefs.h isn't it.

I kind of think there should be a "windows_unpollute.h" header, localized in the windows-OS stuff, that isn't ifdef-guarded, that can be inserted after every instance of a Windows header to protect the rest of the codebase against Windows being Windows. That's obviously out of scope of this change, though if people want me to do it I'm happy to do it, won't take long.

I'm not sure if all the guards I added are necessary. I am sure that several of them are; a single addition was not sufficient.


One thing of note is that the build still fails on iOS and macOS. I believe this is bitrot regarding the testing framework; iOS has a build failure that I suspect is caused by a newer compiler, macOS straight-up doesn't even start building. I suspect these have been fixed in Godot 4.5, but this branch is explicitly a 4.4 branch, and I don't want to start cherrypicking in 4.5 stuff because that's just going to make any ultimate master-branch merge that much more annoying.

I think if the pull request is accepted, the next step is to rebase the entire shebang on top of master and make a libgodot_master branch that will actually merge in cleanly and isn't subject to months-old bitrotting. I am also happy to do this, once the 4.4 branch is in reasonably good shape.

I did some testing but not massive testing; I can vouch for some of the Godot demo projects working fine. I don't have a good way to test the actual libgodot functionality.


You're welcome to squash this or not as you see fit :)


uint32_t hash = hash_murmur3_one_32(has_return);
hash = hash_murmur3_one_32(arguments.size(), hash);
uint32_t compat_hash = hash_murmur3_one_32(has_return);
Copy link

Choose a reason for hiding this comment

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

This is fine to avoid name collision.

@zorbathut
Copy link
Author

Ping @kisg , let me know how this looks!

@nonameentername
Copy link

@zorbathut I was able to get the linux version of libgodot working with your changes but had to make additional changes to get the shared library to compile. You can view my changes here.

@zorbathut
Copy link
Author

@nonameentername Thanks for the patch! Yeah, this wasn't doing a shared library build on Linux, you are correct.

Some questions, though:

  • What's the deal with the -fPIC added to static_library? I don't think this should be necessary but I also don't understand why it's there.
  • What's the deal with the && 0 in servers/display_server.cpp? This seems like at least not the right way to accomplish this :V

(code seems to build fine with both of those reverted, though, so maybe they're just legacy from previous tests?)

The meat of it appears to be the base-prefixed wayland/x11 drivers, and I admit I don't totally understand what's going on here; it's returning a list of source files instead of compiled objects so the parent can compile it with the right commandline flags? Which just kinda happens naturally in platform/linuxbsd/SCsub? That's a guess, please tell me if I'm right!

(This would also suggest that the objects variable can go away entirely, I think?)

If that's right, then that part looks totally fine, though I'll probably isolate it from the other changes; let me know if I'm reading this properly.

@kisg
Copy link

kisg commented Jun 3, 2025

@zorbathut @nonameentername

Thank you for the PR. I started looking through the changes, but I ask for a bit of time, as we are working on some other LibGodot features that we have to finish first this week. (These will be published as well.)

@nonameentername
Copy link

nonameentername commented Jun 3, 2025

@zorbathut The -fPIC flag added to the static_library can be removed—it was part of an earlier attempt to get the static library working, but it didn't end up helping. The && 0 condition was added to prevent an infinite fork loop when Godot is embedded. There's a check that runs Godot internally, and if this section of code isn't excluded, it keeps forking. That condition should probably be replaced with && !LIBGODOT_ENABLED instead.

The changes to SCsub switch from including object files to including source files. This ensures the objects are compiled during the shared or static library step with the correct flags. The objects variable is likely no longer needed and can probably be removed.

@fire
Copy link

fire commented Jun 3, 2025

I took a look and one way I avoid the define hell is to rename MemoryBarrier uniquely to Godot

@zorbathut
Copy link
Author

@nonameentername Cherrypicked with tweaks, thanks for the help!

@zorbathut
Copy link
Author

@fire I'm happy to do this, either in a separate change or added to this one. Any ideas for what to rename it to? Some suggestions:


MemoryAccessBarrier

PipelineMemoryBarrier

MemoryBarrierInfo

AccessBarrier

MemorySyncBarrier


(sadly MemoryBarrier probably is the best option)

Let me know if this is worth doing, it won't take long to make it a search-and-replace, and let me know if you want it as a separate pull request.

@fire
Copy link

fire commented Aug 16, 2025

I have no preference on the name and it'll probably change on maintainer review too.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants