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

Android: Allow using alternative Gradle build directory #88297

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

dsnopek
Copy link
Contributor

@dsnopek dsnopek commented Feb 13, 2024

Currently, when exporting to Android using Gradle (as opposed to using the normal APK template), the build template always comes from res://android

However, this can make exporting difficult, if you have multiple Android export presets and you want to use a custom Godot build for one of them. You need to manually wipe out the res://android/build directory and extract the android_source.zip of your custom Godot build, and then swap back and forth between that and the normal template, when changing which preset you're exporting.

The specific use case I have in mind is if you're making a VR game for, say, Pico and Meta Quest, and want to use a custom Godot build for Meta Quest because you've patched it to fix some platform-specific issue.

Anyway, this PR allows you to configure the top-level directory for the Gradle build, so you could use res://android for Pico and res://quest for the Meta Quest.

It puts the export setting way down at the bottom in a new "Advanced" section, because I don't want to confuse people. However, we do put the fields for the normal APK kind of custom export templates up at the top, so maybe I'm being too paranoid, and this should just go under the "Use Gradle Build" option?

(BTW, you specify the top-level directory like res://android rather than res://android/build because the .build_version file is placed in res://android and this makes sure you have a two level path that allows that.)

It doesn't update the "Project" -> "Install Android Build Template..." feature at all, that will still always extract the template into res://android/build. This may be fine, because this is an advanced feature? It also requires you to make the .build_version file manually, although, I considered just making Godot not check that when customizing the Gradle build dir under the assumption that a developer doing something advanced should know what they are doing?

It seems to work in my testing so far, but I'm not super confident in the implementation or design, so I'm going to start with this as a DRAFT for discussion.

@dsnopek dsnopek added this to the 4.x milestone Feb 13, 2024
@dsnopek dsnopek requested a review from m4gr3d February 13, 2024 20:43
@dsnopek dsnopek requested a review from a team as a code owner February 13, 2024 20:43
@dsnopek dsnopek marked this pull request as draft February 13, 2024 20:43
@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 13, 2024

My current thinking on the approach:

However, we do put the fields for the normal APK kind of custom export templates up at the top, so maybe I'm being too paranoid, and this should just go under the "Use Gradle Build" option?

It's similar to those fields, so we should include it under the Use Gradle Build option as well.
Alongside this new option, we should also include an option that specifies the path to the android_sources.zip to use to populate the gradle build dir.
Until we formalize the feature, let's keep the visibility hidden so it's not accessible to end users, but export plugins can override those settings.

It doesn't update the "Project" -> "Install Android Build Template..." feature at all, that will still always extract the template into res://android/build.

Let's update this setting to use those options instead. So the default behavior will still be the same, but if those options are overridden, then Install Android Build Templates... will do the right thing.

Using your example in #88291 for reference:

  • When the MetaQuestTemplatePlugin is not toggled, then the default (current) behavior applies
  • When the MetaQuestTemplatePlugin is toggled:
    • it overrides the gradle build dir
    • it overrides the path to the android_sources.zip build template
    • the export window shows a warning stating the build template is not installed
    • the user clicks on Install Android Build Templates... to install the custom android_sources.zip into the custom gradle build dir.

@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 13, 2024

Let's update this setting to use those options instead. So the default behavior will still be the same, but if those options are overridden, then Install Android Build Templates... will do the right thing.

The .build_version file should also be updated to include the path of the android_sources.zip build template used to generate it. This will enable us to detect if a custom build template is used for an already existing gradle build dir allowing us to properly notify the user.

@dsnopek dsnopek force-pushed the android-configure-gradle-path branch from bae8664 to 4c0e356 Compare February 14, 2024 18:02
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 14, 2024

Thanks for the review and implementation/design notes!

It's similar to those fields, so we should include it under the Use Gradle Build option as well.
Alongside this new option, we should also include an option that specifies the path to the android_sources.zip to use to populate the gradle build dir.

Ok, changed in my latest push.

Until we formalize the feature, let's keep the visibility hidden so it's not accessible to end users, but export plugins can override those settings.

Sounds good. For now, I've left them visible because it makes testing easier. But I'll make them always hidden before taking this out of draft.

Let's update this setting to use those options instead. So the default behavior will still be the same, but if those options are overridden, then Install Android Build Templates... will do the right thing.

Since you can have multiple export presets with different settings, we'll need a way to pick which one you're installing the build templates for. I'm working on this now...

@dsnopek dsnopek force-pushed the android-configure-gradle-path branch from 4c0e356 to f20b8d1 Compare February 14, 2024 22:07
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 14, 2024

@m4gr3d In my latest push, I think I've addressed all your notes, including the changes to Install Android Build Templates... (with the exception of hiding the new export options, because leaving them visible makes this much easier to test). Please let me know what you think!

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/export/export_template_manager.cpp Show resolved Hide resolved
editor/export/export_template_manager.cpp Outdated Show resolved Hide resolved
platform/android/export/export_plugin.cpp Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the android-configure-gradle-path branch from f20b8d1 to dd6ef49 Compare February 15, 2024 15:06
@dsnopek dsnopek requested a review from m4gr3d February 15, 2024 15:11
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 logic looks good. I'll perform a round of testing on the PR to see what the feature looks like.

editor/editor_node.cpp Outdated Show resolved Hide resolved
editor/export/export_template_manager.h Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the android-configure-gradle-path branch 2 times, most recently from bb5aa7f to f197a1c Compare February 15, 2024 16:40
@dsnopek dsnopek requested a review from m4gr3d February 15, 2024 16:55
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.

@dsnopek Looks pretty good from my testing!

Here are a few feedbacks I have:

  • We should print an export warning (not error) when the value in the .build_version file differs from gradle_build/android_source_template. This way, it's still the responsibility of the user to delete and recreate the build dir, but we provide some hints to the fact it's needed.
  • For a custom gradle_build/android_source_template, we should use the zip file's md5 to ensure we can detect when the file is modified. For example, a plugin may reuse the same location for its custom source template, but update the content for every release.

@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 16, 2024

Until we formalize the feature, let's keep the visibility hidden so it's not accessible to end users, but export plugins can override those settings.

Sounds good. For now, I've left them visible because it makes testing easier. But I'll make them always hidden before taking this out of draft.

Maybe we should introduce an Advanced toggle to make these and the custom template options visible. It's not something I think we should make visible by default as I think it'll confuse users, but I (and I imagine advanced developers) could make use of these features in day-to-day testing.

Just a thought, no need for this to be a blocker for this PR.

@m4gr3d m4gr3d modified the milestones: 4.x, 4.3 Feb 16, 2024
@dsnopek dsnopek force-pushed the android-configure-gradle-path branch from f197a1c to 12bb588 Compare February 16, 2024 20:12
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 16, 2024

Thanks for the feedback! I've made the requested changes in my latest push :-)

Maybe we should introduce an Advanced toggle to make these and the custom template options visible. It's not something I think we should make visible by default as I think it'll confuse users, but I (and I imagine advanced developers) could make use of these features in day-to-day testing.

I added a Show Advanced Options toggle that will reveal the two new Gradle template options:

Selection_117

I don't know that it makes sense to hide the normal custom template options, since those are the first options on every export preset - I think non-advanced users are already used to skipping over them. :-)

Overall, I think we've zero'd in on the approach and implementation here, so I'm going to take this out of draft now!

@dsnopek dsnopek marked this pull request as ready for review February 16, 2024 20:16
@dsnopek dsnopek requested a review from m4gr3d February 16, 2024 20:16
@dsnopek dsnopek changed the title [DRAFT] Android: Allow using alternative Gradle build directory Android: Allow using alternative Gradle build directory Feb 16, 2024
@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 16, 2024

Maybe we should introduce an Advanced toggle to make these and the custom template options visible. It's not something I think we should make visible by default as I think it'll confuse users, but I (and I imagine advanced developers) could make use of these features in day-to-day testing.

I added a Show Advanced Options toggle that will reveal the two new Gradle template options:

This should be a setting at the export preset level (similar to runnable), so we can maintain consistency with the approach we have for the Project Settings. Just looking at the Android export preset, there are a few other options that I'd move under the advanced visibility (e.g: graphics/opengl_debug, command_line/extra_args), and I can see that other platform presets would also benefit from a cleanup as well.

But since it's not related to this PR, this can be tackled in another PR. For now though, I'd remove the Show Advanced Options toggle since the current implementation is only specific to the gradle build settings.

I don't know that it makes sense to hide the normal custom template options, since those are the first options on every export preset - I think non-advanced users are already used to skipping over them. :-)

Existing users may be familiar with them, doesn't mean it's not confusing, especially to new users :)

@dsnopek dsnopek force-pushed the android-configure-gradle-path branch from 12bb588 to 6b79b5b Compare February 16, 2024 20:45
@dsnopek
Copy link
Contributor Author

dsnopek commented Feb 16, 2024

But since it's not related to this PR, this can be tackled in another PR. For now though, I'd remove the Show Advanced Options toggle since the current implementation is only specific to the gradle build settings.

Ok! I've reverted the Show Advanced Options toggle, and gone back to the original plan of just keeping the new Gradle template options hidden for now.

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.

Looks great!

editor/export/export_template_manager.cpp Outdated Show resolved Hide resolved
editor/export/export_template_manager.cpp Outdated Show resolved Hide resolved
@m4gr3d
Copy link
Contributor

m4gr3d commented Feb 16, 2024

Maybe we should introduce an Advanced toggle to make these and the custom template options visible. It's not something I think we should make visible by default as I think it'll confuse users, but I (and I imagine advanced developers) could make use of these features in day-to-day testing.

I added a Show Advanced Options toggle that will reveal the two new Gradle template options:

This should be a setting at the export preset level (similar to runnable), so we can maintain consistency with the approach we have for the Project Settings. Just looking at the Android export preset, there are a few other options that I'd move under the advanced visibility (e.g: graphics/opengl_debug, command_line/extra_args), and I can see that other platform presets would also benefit from a cleanup as well.

But since it's not related to this PR, this can be tackled in another PR. For now though, I'd remove the Show Advanced Options toggle since the current implementation is only specific to the gradle build settings.

I don't know that it makes sense to hide the normal custom template options, since those are the first options on every export preset - I think non-advanced users are already used to skipping over them. :-)

Existing users may be familiar with them, doesn't mean it's not confusing, especially to new users :)

@dsnopek I've added #88419 as a follow up to this comment.

@akien-mga akien-mga merged commit 66b33c1 into godotengine:master Feb 16, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

m4gr3d added a commit to m4gr3d/godot that referenced this pull request Mar 5, 2024
…gradle android source template

Follow-up to godotengine#88297 to address the following issues:
- Ensure that the custom gradle android source template is valid. Show a warning if it's not
- Don't show an error when the official export templates are not installed but a custom android source template is specified
@dsnopek dsnopek deleted the android-configure-gradle-path branch July 22, 2024 15:31
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