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

Fix overriding _export_begin, _export_file and _export_end from GDExtension #80999

Merged

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Aug 25, 2023

Fixes #80593

In EditorExportPlatform, it's explicitly checking for a script before calling these virtual methods on EditorExportPlugin, which prevents them from working correctly when implemented from GDExtension (rather than script).

This PR changes the code to instead check using GDVIRTUAL_IS_OVERRIDDEN_PTR() which will check for both script or GDExtension implementations.

Another (possibly better) way this could have been implemented is by making the default implementations of the virtual _export_begin() etc functions do GDVIRTUAL_CALL() (calling into script or GDExtension), allowing the C++ EditorExportPlugins to override those with custom implementations. It's done this way in a number of other classes. However, when I tried to implement it, there were issues with the fact that the in-engine classes take HashSet<String> p_features whereas the script/GDExtension versions take PackedStringArray, and this would have been recreating the PackedStringArray a lot more times than the current structure. If we changed the in-engine C++ API to take PackedStringArray instead, this would not be a problem!

But for now I've gone with the simpler approach.

NOTE: Currently marked as a draft, because I haven't actually tested it yet! The issue doesn't have an MRP, and there's a bunch of setup to test, but I will do it as soon as I can.

@dsnopek dsnopek added this to the 4.x milestone Aug 25, 2023
@dsnopek dsnopek requested a review from a team August 25, 2023 16:00
@dsnopek dsnopek marked this pull request as draft August 25, 2023 16:08
@dsnopek dsnopek marked this pull request as ready for review August 26, 2023 15:31
@dsnopek dsnopek changed the title [DRAFT] Fix overriding _export_begin, _export_file and _export_end from GDExtension Fix overriding _export_begin, _export_file and _export_end from GDExtension Aug 26, 2023
@dsnopek
Copy link
Contributor Author

dsnopek commented Aug 26, 2023

I've just made a test project (attached below) and verified that this seems to fix the bug! Taking out of draft.

gdext-editor-plugin-test.zip

@dsnopek dsnopek modified the milestones: 4.x, 4.2 Aug 26, 2023
@dsnopek dsnopek added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Aug 26, 2023
@akien-mga akien-mga changed the title Fix overriding _export_begin, _export_file and _export_end from GDExtension Fix overriding _export_begin, _export_file and _export_end from GDExtension Aug 26, 2023
@akien-mga akien-mga merged commit 9a140f9 into godotengine:master Aug 28, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@dsnopek dsnopek deleted the gdextension-editor-export-plugin branch July 22, 2024 15:27
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.

Export plugin not working with GdExtensions
4 participants