Skip to content

[3.x] Disable menus and functionality that are not relevant on the Android Editor port#63085

Merged
akien-mga merged 1 commit intogodotengine:3.xfrom
m4gr3d:update_android_editor_menus_3x
Sep 13, 2022
Merged

[3.x] Disable menus and functionality that are not relevant on the Android Editor port#63085
akien-mga merged 1 commit intogodotengine:3.xfrom
m4gr3d:update_android_editor_menus_3x

Conversation

@m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Jul 16, 2022

For the initial version of the Android editor, we'll only support exporting a pck/zip archive.
As such, the rest of the export functionality is disabled when running on Android since it's not being used.

Polish work for godotengine/godot-proposals#3931

@m4gr3d m4gr3d added this to the 3.6 milestone Jul 16, 2022
@m4gr3d m4gr3d requested review from a team as code owners July 16, 2022 20:12
@m4gr3d m4gr3d changed the title Disable menus and functionality that are not relevant on the Android Editor port [3.x] Disable menus and functionality that are not relevant on the Android Editor port Jul 16, 2022
Copy link
Member

Choose a reason for hiding this comment

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

Why default to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The valid variable is only updated to true when the templates are valid. Since for the Android port, we're skipping the templates' checks, it's never updated to true, and thus always returns false.

For non-Android port, the initial value for valid is always overwritten, so there's no risk of affecting the logic for the other platforms.

Copy link
Member

@akien-mga akien-mga Jul 16, 2022

Choose a reason for hiding this comment

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

Yeah but it seems to be a workaround here. This method checks if export templates are valid. If they're not valid on PC, it should behave the same as if they're not valid on Android. With this change it will be true on Android and false on PC, and the latter seems more accurate to me.

Sounds like what you want to do is just enable PCK export, and the problem here is that the logic to enable it is coupled to the availability of templates. Which is partially true at least for Mono (BCL needs to be included in the PCK), but can also not be a deal-breaker for simple projects.

So what's needed IMO is a bigger refactor that decouples PCK export check from binary export check. Or if a temporary hack is needed for Android, it can simply be not to call can_export since your changes basically makes it a no-op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method checks if export templates are valid. If they're not valid on PC, it should behave the same as if they're not valid on Android. With this change it will be true on Android and false on PC, and the latter seems more accurate to me.

The method unfortunately is a catch-all for export templates validity checks, and project configuration validity checks.
For a full binary export, both checks are valid. However for a pck/zip only export, the export templates validity checks are irrelevant.
So given we'll only (initially) support pck/zip export for the Android editor, the export templates validity checks are being disabled. So yes in this scenario, if the project configuration validity checks fail, they fail both on Android and Windows. If the export templates validity checks fail, they only fail on Windows but not on Android.

Which is partially true at least for Mono (BCL needs to be included in the PCK), but can also not be a deal-breaker for simple projects.

The flow I'm imagining is that a user would do development and testing using the Android editor port, then via export of a zip or pck file, remote versioning or file syncing, continue and finish the release process on a fully featured platform. Would the BCL need to be included for that workflow to work?

Or if a temporary hack is needed for Android, it can simply be not to call can_export since your changes basically makes it a no-op.

The can_export call is still needed for the project configuration validity checks. This particular method is overwritten by the EditorExportPlatformWindows::can_export method which uses this parent logic for its template validation logic, so the short-circuit for Android is still needed.

So what's needed IMO is a bigger refactor that decouples PCK export check from binary export check.

Agree this would be a good approach as well. Splitting can_export into a validate_export_templates and a validate_project_configuration methods. We can then skip the call to the validate_export_templates for the Android editor.

@akien-mga
Copy link
Member

I understand that making Android custom builds on Android would be tricky, but I'm not sure what's the blocker for running the normal export logic for prebuilt APK or desktop platforms?

Some tools like rcedit won't work but it's optional anyway, so I feel users should be able to export a .exe of their game in their Documents folder for later upload.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jul 16, 2022

I understand that making Android custom builds on Android would be tricky, but I'm not sure what's the blocker for running the normal export logic for prebuilt APK or desktop platforms?

Some tools like rcedit won't work but it's optional anyway, so I feel users should be able to export a .exe of their game in their Documents folder for later upload.

For prebuilt apk, we still need to sign the modified apk before it's usable, and the signing tools are not easily available on Android.

For either though I don't think the obstacles are unsurmountable but the approach I'm following is to start with the minimal amount of necessary features and expand based on user feedback; so starting with exporting a pck/zip file, and adding the other export capabilities after they've been tested and validated to work on the Android port.

@m4gr3d m4gr3d force-pushed the update_android_editor_menus_3x branch 2 times, most recently from 8ec5666 to c75e9b3 Compare July 19, 2022 17:25
@m4gr3d m4gr3d changed the title [3.x] Disable menus and functionality that are not relevant on the Android Editor port [3.x] WIP - Disable menus and functionality that are not relevant on the Android Editor port Jul 19, 2022
@m4gr3d m4gr3d force-pushed the update_android_editor_menus_3x branch from c75e9b3 to c61580a Compare August 15, 2022 09:44
@m4gr3d m4gr3d changed the title [3.x] WIP - Disable menus and functionality that are not relevant on the Android Editor port [3.x] Disable menus and functionality that are not relevant on the Android Editor port Aug 15, 2022
@m4gr3d m4gr3d force-pushed the update_android_editor_menus_3x branch from c61580a to 1bc78dc Compare September 9, 2022 04:29
@m4gr3d m4gr3d force-pushed the update_android_editor_menus_3x branch from 1bc78dc to 1f23bac Compare September 13, 2022 14:38
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.

This is ok for the time being but I'm still really not fond of having to hardcode all this.

And I'm still not happy about disabling export functionality, this makes the Android editor port hardly usable if one can't export an APK or a .exe. IMO supporting exporting unsigned APKs or .exe (which should work now I think) would be better than no export at all.

But let's take it incrementally I guess.

@akien-mga akien-mga merged commit 5600be9 into godotengine:3.x Sep 13, 2022
@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.

3 participants