Skip to content

Conversation

@Benjamin-Dobell
Copy link
Contributor

@Benjamin-Dobell Benjamin-Dobell commented Nov 7, 2025

Quite surprised this one managed to go unnoticed for quite some time. The implementation of AssetData's seek was simply performing skip(position), however that's relative to the stream's current position. So it is only accurate if you seek immediately upon opening a file, and you can never seek backwards.

This manifests in some pretty nasty bugs. I ran into this when attempting to load a sparse .pck (the default) from assets://. The C++ implementation naturally needs to seek toward the end of the file to obtain the directory mapping. However, this fails and we overshoot the intended location and the file count read is then incorrect, as is everything else that follows.

@Benjamin-Dobell Benjamin-Dobell requested a review from a team as a code owner November 7, 2025 12:50
@AThousandShips AThousandShips changed the title Android: Fixed assets:// seek() major bug i.e. fixed loading sparse .pck from assets:// Android: Fix loading sparse .pck from assets:// Nov 7, 2025
@AThousandShips AThousandShips added this to the 4.6 milestone Nov 7, 2025
@akien-mga akien-mga added the cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release label Nov 7, 2025
@bruvzg
Copy link
Member

bruvzg commented Nov 7, 2025

When PCK is loaded using this code? Usually it is loaded as res://assets.sparsepck and handled by AAssetManager/FileAccessAndroid. And assets:// seems to be only used for Android Editor keystores.

@Benjamin-Dobell
Copy link
Contributor Author

Yeah, I'm honestly not too sure how often people typically try load .pck from assets://. I guess this will impact any file loaded from assets://, so if users are doing that for some reason and attempt to seek, that'll fail too. Again, presumably not that common or else this would have been raised before.

As to why I ran into this myself. I was setting up native Android debugging for GodotJS. I'm using --main-pack as quick and easy way to test my Godot projects.

@AThousandShips
Copy link
Member

I think it'd be good to have a bug report to discuss the specific conditions and use case where this would happen

@bruvzg
Copy link
Member

bruvzg commented Nov 7, 2025

The change looks good. Probably was not noticed since keystore is always read/written in one chunk. It's not really related to PCK, since anything using seek should fail.

But I'm not sure what was the purpose of adding assets:// in the first place, and whether it is supposed to be user accessible.

@akien-mga akien-mga removed the cherrypick:4.5 Considered for cherry-picking into a future 4.5.x release label Nov 7, 2025
@m4gr3d
Copy link
Contributor

m4gr3d commented Nov 7, 2025

But I'm not sure what was the purpose of adding assets:// in the first place, and whether it is supposed to be user accessible.

The assets:// path provides a mean for editor build to access their own assets directory, since for editor builds, res:// points to the general filesystem where the loaded project is stored.
On template builds, res:// and assets:// point to the same location, but are handled by different logic, so on template builds, using res:// is the recommended approach.

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