-
Notifications
You must be signed in to change notification settings - Fork 24
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
Migrate the export scripts from gdscript to C++ via gdextension #61
Migrate the export scripts from gdscript to C++ via gdextension #61
Conversation
698fcf0
to
491ba88
Compare
491ba88
to
16fb762
Compare
I wish we fixed the enabling of plugin logic, it feels like a dirty workaround :) That said, I think this logic works much better as a GDExtension anyway. My only "concern" is that it's less accessible, it'll be harder to find contributors who want to work on this. Though that's more an issue for plugins like XR tools than this. |
@@ -90,8 +90,14 @@ jobs: | |||
- name: Create Godot-CPP library | |||
run: | | |||
cd aar/thirdparty/godot-cpp | |||
scons platform=${{ matrix.platform }} target=template_debug ${{ matrix.flags }} | |||
scons platform=${{ matrix.platform }} target=template_release ${{ matrix.flags }} | |||
scons platform=${{ matrix.platform }} target=template_debug arch=arm64 |
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 should be handled through the matrix, adding the different variations. Not all platforms support these archs, so this will break on MacOS
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.
All those combinations are only for the android
platform (see if: matrix.platform == 'android'
below).
This ensures we generate the shared libraries for all supported android
architectures prior to running the gradle build.
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.
Ah missed that, sorry :) that makes perfect sense!
Approving this seeing it makes sense to move forward with this, but I'm still on the fence if we should have an extension per platform. It makes far more sense to me to have a single extension so that logic is properly shared and not registered multiple times with the IDE especially as vendors adopt each others extensions. We don't need separate plugins to have separate export classes. |
16fb762
to
3d9cdde
Compare
Unfortunately, so long as each vendor has its own openxr loader, it's not possible to have a single gdextension for all vendors since the loader shared libraries will conflict with each other. |
The GDExtension just contains the editor and extension classes, it doesn't limit having separate folders with the loaders and picking the right loader to install right? Or is there something that prevents us from having multiple editor plugin classes in a single extension library? I don't see anything in the current source that would prevent us from combining all into a single extension, we still have the separate loader folders for the AARs off course. Or am I missing something? |
The extension classes have vendor specific header dependencies (example) which prevents them from being easily combined. The editor plugin classes could be in a single extension library since they don't have dependencies on the loaders, this would require them to be compiled separately from the extension classes however which would defeat the point of having a single extension library. |
3d9cdde
to
f303bc7
Compare
This removes the need to enable the plugin before it's usable in the export preset settings.