-
Notifications
You must be signed in to change notification settings - Fork 2
Build and test fixes. #3
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
Build and test fixes. #3
Conversation
…eviceDriver::MemoryBarrier.
|
|
||
| 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); |
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.
This is fine to avoid name collision.
|
Ping @kisg , let me know how this looks! |
|
@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. |
|
@nonameentername Thanks for the patch! Yeah, this wasn't doing a shared library build on Linux, you are correct. Some questions, though:
(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 (This would also suggest that the 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. |
|
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.) |
|
@zorbathut The The changes to |
|
I took a look and one way I avoid the define hell is to rename MemoryBarrier uniquely to Godot |
|
@nonameentername Cherrypicked with tweaks, thanks for the help! |
|
@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. |
|
I have no preference on the name and it'll probably change on maintainer review too. |
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_filterinto 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:
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
masterand make alibgodot_masterbranch 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 :)