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

[4.0] Platform export refactoring #50771

Merged

Conversation

naithar
Copy link
Contributor

@naithar naithar commented Jul 23, 2021

Previous PR: #50467

Another approach at export refactoring proposal: godotengine/godot-proposals#2929 that was suggested there.

Allows to use .cpp files other than export.cpp for platform's export template. Keeps current structure where export folder and it's code is kept with platform.

export.cpp files for each platform are also separated into more files for initial refactoring.
Future PRs could bring more changes to exports to cleanup the code and make it easier to navigate.

@naithar naithar requested review from a team as code owners July 23, 2021 12:09
@Calinou Calinou added this to the 4.0 milestone Jul 23, 2021
Copy link
Contributor

@m4gr3d m4gr3d left a comment

Choose a reason for hiding this comment

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

The Android section looks good!

@akien-mga
Copy link
Member

Needs a rebase, otherwise seems good to merge.

@naithar naithar force-pushed the feature/platform-export-refactoring-4.0 branch from 67f60f0 to 253581f Compare August 12, 2021 14:24
@akien-mga akien-mga merged commit 6f043f7 into godotengine:master Aug 12, 2021
@akien-mga
Copy link
Member

Thanks!

Comment on lines +313 to +315
} else if (mode == EXPORT_MODE_GDNATIVE) {
r_features->push_back("wasm32");
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason to not just always have wasm32 as an export feature for the JavaScript platform?

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.

5 participants