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

Migrate the export scripts from gdscript to C++ via gdextension #61

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

m4gr3d
Copy link
Collaborator

@m4gr3d m4gr3d commented Nov 30, 2023

This removes the need to enable the plugin before it's usable in the export preset settings.

@m4gr3d m4gr3d added the enhancement New feature or request label Nov 30, 2023
@m4gr3d m4gr3d added this to the 2.1.0 milestone Nov 30, 2023
@m4gr3d m4gr3d force-pushed the move_export_scripts_to_gdextension branch from 698fcf0 to 491ba88 Compare November 30, 2023 06:24
@m4gr3d m4gr3d marked this pull request as ready for review November 30, 2023 17:38
@m4gr3d m4gr3d force-pushed the move_export_scripts_to_gdextension branch from 491ba88 to 16fb762 Compare December 3, 2023 19:22
@BastiaanOlij
Copy link
Member

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
Copy link
Member

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

Copy link
Collaborator Author

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.

Copy link
Member

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!

SConstruct Outdated Show resolved Hide resolved
@BastiaanOlij
Copy link
Member

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.

@m4gr3d m4gr3d force-pushed the move_export_scripts_to_gdextension branch from 16fb762 to 3d9cdde Compare December 4, 2023 02:51
@m4gr3d
Copy link
Collaborator Author

m4gr3d commented Dec 4, 2023

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.

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.

@BastiaanOlij
Copy link
Member

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?

@m4gr3d
Copy link
Collaborator Author

m4gr3d commented Dec 6, 2023

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.

@m4gr3d m4gr3d force-pushed the move_export_scripts_to_gdextension branch from 3d9cdde to f303bc7 Compare December 8, 2023 05:38
@m4gr3d m4gr3d merged commit f384632 into GodotVR:master Dec 8, 2023
5 checks passed
@m4gr3d m4gr3d deleted the move_export_scripts_to_gdextension branch December 8, 2023 07:29
@BastiaanOlij BastiaanOlij modified the milestones: 2.1.0, 2.0.3 Jan 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants