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

New API for GDNative #44989

Merged
merged 2 commits into from
Jan 25, 2021
Merged

New API for GDNative #44989

merged 2 commits into from
Jan 25, 2021

Conversation

vnen
Copy link
Member

@vnen vnen commented Jan 7, 2021

This removes the old functions that were wrappers for builtin functions and instead provide the new discovery API to get function pointers for those.

Also removes the print*() functions since those are utility functions that also can be queried.

@vnen vnen added this to the 4.0 milestone Jan 7, 2021
@vnen vnen requested a review from bruvzg as a code owner January 7, 2021 12:58
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Amazing cleanup. I like PRs with this kind of additions/removals ratio :)

modules/gdnative/gdnative/packed_arrays.cpp Show resolved Hide resolved
modules/gdnative/gdnative/packed_arrays.cpp Show resolved Hide resolved
modules/gdnative/gdnative/callable.cpp Show resolved Hide resolved
modules/gdnative/gdnative/color.cpp Outdated Show resolved Hide resolved
modules/gdnative/gdnative/rid.cpp Show resolved Hide resolved
modules/gdnative/gdnative/signal.cpp Show resolved Hide resolved
modules/gdnative/gdnative/string.cpp Show resolved Hide resolved
modules/gdnative/gdnative/string_name.cpp Outdated Show resolved Hide resolved
modules/gdnative/include/gdnative/basis.h Show resolved Hide resolved
@akien-mga
Copy link
Member

Windows build seems to fail with:

D:\a\godot\godot\modules\gdnative/gdnative.h(40): fatal error C1083: Cannot open include file: 'gdnative_api_struct.gen.h': No such file or directory

@akien-mga
Copy link
Member

Aside from gdnative/variant.cpp, all .cpp are now pretty short and simple (new + sometimes destroy), there could be a case for grouping them all in some gdnative/variant_bind.cpp or similar à la core... but that's also arguably a bad habit that we have to make giganormous .cpp files of related components, so it might be fine to keep as is ;)

@vnen
Copy link
Member Author

vnen commented Jan 8, 2021

Windows build seems to fail with:

D:\a\godot\godot\modules\gdnative/gdnative.h(40): fatal error C1083: Cannot open include file: 'gdnative_api_struct.gen.h': No such file or directory

I'm not sure why this is happening. Is it failing to generate this file? Reminds of the old issue with parallel builds on Windows, but I think that was fixed already.

Edit: tried on my machine and it builds fine.

@vnen vnen force-pushed the gdnative-new-api branch 3 times, most recently from a8d0b3c to af1f403 Compare January 12, 2021 12:38
@vnen
Copy link
Member Author

vnen commented Jan 12, 2021

Regarding the build issue, I've exhausted my ideas. It seems the WebRTC module needs GDNative but for some reason it doesn't trigger the generation of the API struct header. On my machine it works. I wonder if it's something related to the CI cache.

@akien-mga
Copy link
Member

akien-mga commented Jan 12, 2021

Regarding the build issue, I've exhausted my ideas. It seems the WebRTC module needs GDNative but for some reason it doesn't trigger the generation of the API struct header. On my machine it works. I wonder if it's something related to the CI cache.

It does call

build_gdnative_api_struct(["modules\gdnative\include\gdnative_api_struct.gen.h", "modules\gdnative\gdnative_api_struct.gen.cpp"], ["modules\gdnative\gdnative_api.json"])

but it seems that there's a race condition, the file is not generated/released in time to be used by the webrtc gdnative code.

I'd posit that the race condition is likely a pre-existing bug (the dependency from modules/webrtc/ on the generation of modules\gdnative\include\gdnative_api_struct.gen.h is not registered properly) which is exposed by this PR which modifies only GDNative and therefore can reuse most of the cache. So the build can very quickly start both the file generation and the webrtc gdnative builds in parallel, which fails (while it might work on clean builds as the file generation would likely happen much earlier).

CC @Faless

@Faless
Copy link
Collaborator

Faless commented Jan 12, 2021

@akien-mga is there any way I can tell scons that one module depends on the other so they are built sequentially?

@akien-mga
Copy link
Member

@Faless No easy way that I can think of, at least with our current setup. The build order likely comes from how the SCsub files are called in modules/SCsub, so we might need to hardcode some hacks there to make sure that GDNative comes last - I think the build order is reversed compared to the calling order for the SConscript (see e.g. how the folders are listed in the main SConscript, core is listed first and built last.

Current order for modules seems to be alphabetical.

@akien-mga
Copy link
Member

akien-mga commented Jan 12, 2021

Testing this hack, let's see: akien-mga@5b0e522

Edit: No luck. But now it failed while building tests early on, doesn't seem related to webrtc. It's not clear to me what file caused the include of the GDNative generated header, but it seems that tests/SCsub also includes GDNative, which might cause issues. So #41976 (review) might be part of the problem.

@vnen
Copy link
Member Author

vnen commented Jan 24, 2021

I updated this to add a version of the discovery API to take a regular const char * instead of a godot_string_name *. For the C++ bindings (and probably others too) it's pretty useful not need to create a StringName for each call.

I've also added a few missing methods to the builtin methods discovery (to get argument count, type and name, and the return type, besides checking if they are const or vararg).

@vnen
Copy link
Member Author

vnen commented Jan 24, 2021

But it's still failing to build. I will try to take a look in the tests.

This API now uses the discovery functions present in Variant instead of
wrapping every built-in function. Users now need to query for function
pointers and use those.
Those are now utilities so the function pointer can be fetched when
needed.
@vnen
Copy link
Member Author

vnen commented Jan 25, 2021

I think I fixed this now. Let's see if the CI finishes correctly.

Edit: Yes, it did. Somehow only on the Windows editor build it was wrongly treating #include "gdnative/gdnative.h" in test_variant.h as modules/gdnative/gdnative.h instead of the correct modules/gdnative/include/gdnative/gdnative.h. So I changed it to #include <gdnative/gdnative.h> which is resolved correctly.

@akien-mga akien-mga merged commit 7f0b5a3 into godotengine:master Jan 25, 2021
@akien-mga
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants