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

[3.x] Add full support for Android scoped storage. #51815

Merged
merged 1 commit into from
Jun 27, 2022

Conversation

m4gr3d
Copy link
Contributor

@m4gr3d m4gr3d commented Aug 17, 2021

This was done by refactoring directory and file access handling for the Android platform so that any general filesystem access type go through the Android layer.

This allows us to validate whether the access is unrestricted, or whether it falls under scoped storage and thus act appropriately.

Fix #38913
Fix #45467
Fix #48636

@m4gr3d m4gr3d added this to the 3.4 milestone Aug 17, 2021
@m4gr3d m4gr3d requested a review from a team August 17, 2021 22:00
@m4gr3d m4gr3d requested a review from a team as a code owner August 17, 2021 22:00
@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch 4 times, most recently from 4e87aa5 to 06828a7 Compare August 18, 2021 00:34
@HEAVYPOLY
Copy link
Contributor

HEAVYPOLY commented Oct 22, 2021

Does this allow read/write external access on Android 11 equivalent to Android 10 "use_legacy_storage" option?
My Android 11 users have lost access to all their painting files and hoping this might fix it.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 25, 2021

Does this allow read/write external access on Android 11 equivalent to Android 10 "use_legacy_storage" option? My Android 11 users have lost access to all their painting files and hoping this might fix it.

Yes it should. Granted this still needs to go through testing in order to validate the behavior.

@m4gr3d m4gr3d modified the milestones: 3.4, 3.5 Oct 25, 2021
@HEAVYPOLY
Copy link
Contributor

Does this allow read/write external access on Android 11 equivalent to Android 10 "use_legacy_storage" option? My Android 11 users have lost access to all their painting files and hoping this might fix it.

Yes it should. Granted this still needs to go through testing in order to validate the behavior.

Ok, I'll test it today, thanks!

@HEAVYPOLY
Copy link
Contributor

HEAVYPOLY commented Oct 26, 2021

Gradle 7.2

Build time: 2021-08-17 09:59:03 UTC
Revision: a773786b58bb28710e3dc96c4d1a7063628952ad

Kotlin: 1.5.21
Groovy: 3.0.8
Ant: Apache Ant(TM) version 1.10.9 compiled on September 27 2020
JVM: 11.0.13 (Eclipse Adoptium 11.0.13+8)
OS: Mac OS X 11.6 x86_64

I'm getting this warning:

Task :java:lib:compileReleaseKotlin
w: /Users/kakapoopie/godot/platform/android/java/lib/src/org/godotengine/godot/io/StorageScope.kt: (83, 32): 'getExternalStorageDirectory(): File!' is deprecated. Deprecated in Java
w: /Users/kakapoopie/godot/platform/android/java/lib/src/org/godotengine/godot/io/StorageScope.kt: (94, 18): 'getExternalStoragePublicDirectory(String!): File!' is deprecated. Deprecated in Java
w: /Users/kakapoopie/godot/platform/android/java/lib/src/org/godotengine/godot/io/StorageScope.kt: (97, 18): 'getExternalStoragePublicDirectory(String!): File!' is deprecated. Deprecated in Java
w: /Users/kakapoopie/godot/platform/android/java/lib/src/org/godotengine/godot/io/file/MediaStoreData.kt: (96, 37): 'getExternalStorageDirectory(): File!' is deprecated. Deprecated in Java

Which version of java should I be using?

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 26, 2021

@HEAVYPOLY Your java version is fine; the deprecation warnings are expected, and shouldn't affect the build process.
Do they prevent you from completing the build?

@HEAVYPOLY
Copy link
Contributor

HEAVYPOLY commented Oct 26, 2021

It built successfully, but program crashes after splash screen :/ This is on Android 10 device (Samsung note 9)

also see this in build:

Task :java:lib:compileDebugJavaWithJavac
Note: Some input files use or override a deprecated API.
Note: Recompile with -Xlint:deprecation for details.

@m4gr3d
Copy link
Contributor Author

m4gr3d commented Oct 26, 2021

@HEAVYPOLY Can you send me the crash stack traces, I'll take a look.

@HEAVYPOLY
Copy link
Contributor

HEAVYPOLY commented Oct 26, 2021

@HEAVYPOLY Can you send me the crash stack traces, I'll take a look.

https://pastebin.com/CkPinEr6
Is this what you need? Not sure how to get stack trace

Also just tried with a friend's Android 11 device and got the crash after splash screen (Samsung note 20)

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch 3 times, most recently from 07ba04a to 182c35b Compare December 21, 2021 23:42
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Dec 27, 2021

@HEAVYPOLY I fixed the issues causing the crash and rebased the PR.

I did a quick test, and starting and loading a Godot app seems to work as expected. I still need to fully validate scoped storage, and I'm working to complete that later this week.

@HEAVYPOLY
Copy link
Contributor

@m4gr3d thanks! Look forward to testing, this is huge!

@ondesic
Copy link

ondesic commented Dec 28, 2021

Please let me know when this can be tested. I am excited to test this as well.

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from 729320c to 43acc60 Compare June 9, 2022 19:13
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jun 10, 2022

@m4gr3d do I need to compile the version myself, or is there one I can get already compiled?

@ondesic Do you need a build of the Godot Editor, or just of the Godot Android libraries?

@ondesic
Copy link

ondesic commented Jun 10, 2022

@m4gr3d The project I am using for a test is a Mono project. Would it be possible to get a mono editor build with the export templates to test this?

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from 603d3d7 to e3bd05f Compare June 22, 2022 16:36
@akien-mga
Copy link
Member

akien-mga commented Jun 23, 2022

Needs a rebase after cherry-pick of #62297 (which in retrospective could also have been merged only for 3.4, but well just in case we don't get to merge this in time for 3.5 due to unforeseen issues).

@akien-mga
Copy link
Member

akien-mga commented Jun 23, 2022

I made a test build with the official buildsystem upgraded to use NDK r23 (#61691) and including this PR to add full support for Android scoped storage.

Could you test @HEAVYPOLY @ondesic @m4gr3d? There's both Android templates and the desktop editor builds as you need the same version to export.
https://downloads.tuxfamily.org/godotengine/testing/3.5-rc-android-scoped%2bndk23/

If I get some confirmations that this work as intended quickly, then we should be able to include it in 3.5-stable too (first in 3.5 RC 5).

@akien-mga
Copy link
Member

@m4gr3d I've got a report that audio doesn't seem to work in my test build, while it works in 3.5 RC 4: https://twitter.com/MagellanicGames/status/1540251677772730368

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from e3bd05f to b00c992 Compare June 24, 2022 15:49
@MagellanicGames
Copy link

Hi, I was asked to provide a repro for the audio bug. I've tested this in the build @akien-mga provided and it will play the sound effect upon pressing the button on desktop but not on the Android device.

The android device is a Samsung Galaxy s6, running Android v7.0.

I found the bug in my game as the code is used for triggering effects for all manner of entites, whether they be UI, enemies etc.

Apologies that it's just smashed together, it is ripped from my production code and didn't have long to get it done.
This is code (for better or worse) that I have used since Godot v3.0.

Hope it is of use.

audio_bug.zip

@akien-mga
Copy link
Member

I can confirm the regression with @MagellanicGames' reproduction project.

The Audio.gd script has some custom logic to parse the files in res://Audio/Sfx/ (which when exported are only .import files) to infer the names of the original WAV files which can then be load()ed. That doesn't seem to work with this PR.
Loading with load("res://Audio/Sfx/item_pickup.wav") works fine however.

It seems like it boils down to Directory.open() not working for res:// paths on Android.
This minimal project fails too (from the Directory docs):

func dir_contents(path):
	var contents = []
	var dir = Directory.new()
	if dir.open(path) == OK:  # Fails here on Android with this PR.
		dir.list_dir_begin()
		var file_name = dir.get_next()
		while file_name != "":
			if dir.current_is_dir():
				print("Found directory: " + file_name)
			else:
				print("Found file: " + file_name)
			contents.append(file_name)
			file_name = dir.get_next()
	else:
		print("An error occurred when trying to access the path.")
	return contents


func _ready():
	# Should find ".." and "." directories, "hello" and "world" files.
	# On Android, fails opening the folder so doesn't find anything.
	$TextEdit.text = str(dir_contents("res://somedir"))

MRP: bug_scoped_storage_open.zip

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from b00c992 to d6efa13 Compare June 25, 2022 23:58
@m4gr3d
Copy link
Contributor Author

m4gr3d commented Jun 26, 2022

@akien-mga @MagellanicGames Thanks for the repro instructions!

I found and fixed the issue, so this should be good to go now!

@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from d6efa13 to 15b59e4 Compare June 26, 2022 16:40
This was done by refactoring directory and file access handling for the Android platform so that any general filesystem access type go through the Android layer.
This allows us to validate whether the access is unrestricted, or whether it falls under scoped storage and thus act appropriately.
@m4gr3d m4gr3d force-pushed the refactor_android_storage_handling branch from 15b59e4 to 24e3b3b Compare June 26, 2022 23:54
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.

Looks great to me!

Tested on Poco F1 (MIUI 12 based on Android 10), I could read from res://, write to and read from user://, and write to and read from Documents (using Manage Documents). Didn't test external storage.

@akien-mga akien-mga merged commit ae60597 into godotengine:3.x Jun 27, 2022
@akien-mga
Copy link
Member

Thanks!

@ondesic
Copy link

ondesic commented Jun 28, 2022

Just tested my mono project and it is crashing. Says "Permission to access file: /storage/emulated/0/Documents/Data/Custom.txt is denied"

Also, I changed my Target SDK to 32 to allow RC_5 to run. I will try to create a test

@ondesic
Copy link

ondesic commented Jun 28, 2022

Here are my findings with the included test mono project.
This app is coded to take a file stored in "res://Test/Test.txt" and create a folder named "Test" in the Document folder, then copy the Test.txt file into that folder. When you press the big button, it will read the Test.txt file and assign the string in the file to the button's text.
This runs as expected in Windows. However, on the android device:

  1. if I select "Manage Documents" only, the app will create a "Test" folder but will NOT copy the Test.txt file. I end up with an empty folder.
  2. Next, I uninstall the app and delete the Test folder. I now add "Manage External Storage". The same thing happens, only the Test folder is created with no file inside.
  3. Next I uninstalled and unchecked all permissions. I then only checked "Read External Storage" and "Write External Storage". I got the same results...Test folder created but empty and unable to copy Test.txt to the folder.
  4. Finally, No combination worked except checking all of the following "Manage External Storage", "Read External Storage" and "Write External Storage". Then I have to allow the permissions for "All Files" and then it will finally allow me to copy the Test.txt file..........except...... it makes Test.txt a 0 byte file with nothing inside.
    Here is the project.
    TestStorage.zip

side note: I even tried to copy the Test.txt file manually to the Test folder. Pressing the button in the app should read the content but it still won't work.

@madmiraal
Copy link
Contributor

@ondesic To ensure this is properly captured and addressed, you should create a new issue.

@m4gr3d m4gr3d deleted the refactor_android_storage_handling branch July 5, 2022 16:48
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