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: Add option to always use WiFi to connect to remote debug #79504

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

Pingar5
Copy link
Contributor

@Pingar5 Pingar5 commented Jul 15, 2023

This PR adds two editor settings for Android which allow you to used Wi-Fi remote debugging on devices with API level >= 21. This is specifically useful in the project I am working on which is a real-time AR game in which GPS is the primary method of player input. This allows our team to keep debugging enabled without needing to be tethered to the computer which is debugging the application.

Please let me know if there is anything I can do to improve the quality of the code!

@Pingar5 Pingar5 requested review from a team as code owners July 15, 2023 15:16
@Pingar5 Pingar5 marked this pull request as draft July 15, 2023 16:21
@Pingar5 Pingar5 marked this pull request as ready for review July 15, 2023 18:08
@Pingar5
Copy link
Contributor Author

Pingar5 commented Jul 15, 2023

Can anyone assist me in determining next steps for fixing the workflow? I don't understand what I've done wrong

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 changes look good!

Were you able to validate the functionality and check it doesn't regress on the remote debug functionality?

@Pingar5
Copy link
Contributor Author

Pingar5 commented Jul 17, 2023

I have validated the functionality works both on LAN and over WAN, but I have never been able to get remote debug over USB to work on my PC and I don't have an android device with API level <= 21 so I haven't done any regression testing

@YuriSizov YuriSizov modified the milestones: 4.x, 4.2 Jul 25, 2023
@YuriSizov YuriSizov changed the title Added option to always use wifi to connect to remote debug Add an option to always use WiFi to connect to remote debug Jul 25, 2023
@YuriSizov
Copy link
Contributor

Could you please squash your commits into one? Make sure that the final commit has a short but descriptive message (the title of this PR is a good option). See this documentation, if you need help with squashing.

@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 26, 2023

For the record, you haven't squashed commits. You only merged the upstream master into your branch. You need to rebase against the master, not merge it into your branch. And as you do the rebase you can do the squashing. Please refer to the linked documentation.

@Pingar5
Copy link
Contributor Author

Pingar5 commented Jul 26, 2023

Yeah sorry, I'm not very good at git and ran out of time. I'll come back to it in a day or two and do it right

@akien-mga akien-mga changed the title Add an option to always use WiFi to connect to remote debug Android: Add option to always use WiFi to connect to remote debug Aug 2, 2023
@akien-mga
Copy link
Member

akien-mga commented Aug 2, 2023

I force pushed an update to squash the commits. If you want to edit this PR further, you would need to reset your local branch to match your now rebased remote branch, with e.g.:

git remote add fork git@github.com:Pingar5/godot
git fetch fork
git checkout master
git reset --hard fork/master

Then do further changes with git commit --amend and git push --force.

For future PRs, you may want to create a dedicated branch for the PR instead of using your fork's master branch, which makes it easier to manage multiple PRs in parallel, and slightly easier to do rebases without messing it up.

Comment on lines 144 to 145
if (EDITOR_GET("export/android/use_wifi_for_remote_debug")) {
host = EDITOR_GET("export/android/wifi_remote_debug_host");
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit concerned about that check, as it would fail if export/android/use_wifi_for_remote_debug doesn't exist. This could happen if someone does a custom build without Android support for one reason or another. The platform code is meant to stay modular so that it can be easily swapped for other platforms. In this case it's unlikely to ever be a problem for Android, but this slightly breaks the encapsulation.

Technically, this check would not be needed if the Android remote debug host was configured via network/debug/remote_host. Is there any issue with this approach, aside from it separating the setting to force override WiFi debug, and the actual debug configuration?

Copy link
Member

Choose a reason for hiding this comment

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

CC @bruvzg

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 reason I didn't use network/debug/remote_host is because currently that field is a dropdown which only gives the options for internal IPs assigned to the computer running the editor. For my use-case I need to put in the external IP as we sometimes walk out of the range of the LAN while testing

Copy link
Member

@akien-mga akien-mga Aug 17, 2023

Choose a reason for hiding this comment

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

I guess we can go ahead with the current approach, but you should check that the settings exist otherwise it will show an error if for whatever reason Godot is compiled with the Android platform:

if (EditorSettings::get_singleton()->has_setting("export/android/use_wifi_for_remote_debug") && EDITOR_GET("export/android/use_wifi_for_remote_debug")) {

Copy link
Member

Choose a reason for hiding this comment

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

Did that change myself.

@akien-mga akien-mga requested a review from bruvzg August 2, 2023 09:31
@Pingar5
Copy link
Contributor Author

Pingar5 commented Aug 2, 2023

I force pushed an update to squash the commits. If you want to edit this PR further, you would need to reset your local branch to match your now rebased remote branch, with e.g.:

For future PRs, you may want to create a dedicated branch for the PR instead of using your fork's master branch, which makes it easier to manage multiple PRs in parallel, and slightly easier to do rebases without messing it up.

Thanks for the help! I will make sure to use a branch next time

@akien-mga akien-mga merged commit 120abd1 into godotengine:master Aug 28, 2023
@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.

6 participants