Fix "res://" being replaced by resource packs in the editor and on Android

[tracefree]

The problem

When using ProjectSettings.load_resource_pack and launching the game from the editor or on Android, DirAccess no longer sees files and directories outside of loaded packs, which is unexpected behavior and makes it difficult to work on modding support for example.

My solution

The simplest way I could find to solve this was to add a new kind of PackSource: PackedSourceDirectory (working title).
It uses a resource directory as the source of the "pack" rather than a file, and returns regular FileAccess references.
The first time a resource pack is loaded, the entire "res://" directory gets added to PackedData.

I adapted the MRP of #48563 to Godot 4.x:
MRPResourcePack.zip

I am new to Godot's codebase and C++ in general, so there is probably a better solution. I'd be happy about any feedback or suggestions on how to solve this problem properly.

Edit: This should also solve replace_files == false not doing anything in non-exported projects, see #89299.

• Bugsquad edit, fixes: ProjectSettings.load_resource_pack replaces res:// completely #48563
• Bugsquad edit, fixes: Issues with ProjectSettings.load_resource_pack(pack) when running the project from the editor #19815
• fixes After loading a pck file (including zip), you can only access the directories and files in the pck #87274
• fixes It is currently impossible to test external package loading from within the editor #16798
• fixes ProjectSettings.load_resource_pack overrides files in the main project if replace_files is false. #89299

[tracefree]

One alternative approach I could imagine might be preferable is to change the behavior of DirAccess::make_default, but I think that would require more substantial changes and I'm not sure I understand the codebase well enough to make them myself.

[scgm0]

Similar problems also exist on Android, because on Android, "res://" is also a real local directory.

[tracefree]

Similar problems also exist on Android, because on Android, "res://" is also a real local directory.

Is there an issue documenting the problem on Android? I'm not sure my fix would resolve that, but I could try it out.

By the way, this may go beyond the scope of a bugfix, but I think my approach could easily be adapted to allow users to load any directory on the filesystem as a "pack", not just the resource folder. This could be very useful to modders, who would otherwise need to pack all assets into a .pck or .zip file every time they want to test changes to their mod while developing it. This would work by e.g. ProjectSettings.load_resource_pack("path/to/mod/directory").

[scgm0]

Similar problems also exist on Android, because on Android, "res://" is also a real local directory.

Is there an issue documenting the problem on Android? I'm not sure my fix would resolve that, but I could try it out.

By the way, this may go beyond the scope of a bugfix, but I think my approach could easily be adapted to allow users to load any directory on the filesystem as a "pack", not just the resource folder. This could be very useful to modders, who would otherwise need to pack all assets into a .pck or .zip file every time they want to test changes to their mod while developing it. This would work by e.g. ProjectSettings.load_resource_pack("path/to/mod/directory").

#87274
Although I didn’t mention Android in the introduction, I did initially discover this problem on Android.

[tracefree]

#87274 Although I didn’t mention Android in the introduction, I did initially discover this problem on Android.

Ah yes, that does appear to be the same problem and my PR should fix it (unless there are additional complications due to Android, I'll try to test that out). Adding it to the list of relevant issues, thank you.

[tracefree]

I just realized there is a problem in my code: After loading a resource pack, you can only see files that existed in res:// at the time the first pack was loaded. Meaning any files that were created in the resource folder at runtime after loading the pack won't be visible. This is still better than the current behavior, where you can't see any files not belonging to datapacks in the resource folder at all, but ideally there shouldn't be any unexpected side effects to loading a resource pack.
I'll try to see if there's a simple way to keep the contents of PackedData synced to changes in the filesystem. This would probably have to involve adding something like "remove_path" to PackedData to support cases where resource files were removed at runtime.

[scgm0]

I just realized there is a problem in my code: After loading a resource pack, you can only see files that existed in res:// at the time the first pack was loaded. Meaning any files that were created in the resource folder at runtime after loading the pack won't be visible. This is still better than the current behavior, where you can't see any files not belonging to datapacks in the resource folder at all, but ideally there shouldn't be any unexpected side effects to loading a resource pack. I'll try to see if there's a simple way to keep the contents of PackedData synced to changes in the filesystem. This would probably have to involve adding something like "remove_path" to PackedData to support cases where resource files were removed at runtime.

It feels like the current fix is the usable version, res:// is not supposed to be modified at runtime, so synchronizing only the first time is good enough.

[tracefree]

It feels like the current fix is the usable version, res:// is not supposed to be modified at runtime, so synchronizing only the first time is good enough.

I tend to agree, but then the behavior of this edge case should be documented. Fact is that it is currently possible to modify res:// at runtime in the editor, and while it doesn't make sense to do so in exported builds I could imagine there are legitimate use cases for it during development of a project. Perhaps I could add a short sentence to the documentation of load_resource_pack?

[tracefree]

The last thing I need clarity on is whether and how to expose PackedSourceDirectory to users. At the moment, due to how I implemented the fix, users can call load_resource_pack("res://") by themselves, and they can even use subdirectories of res://, which would be like loading a pack that has the contents of that directory. I see four options:

1. Do nothing. Then there would be undocumented / possibly undefined behavior of load_resource_pack.
2. Leave it as is but document this relatively useless behavior.
3. Somehow try to prevent users from doing this manually.
4. Make it into a useful feature by allowing to load any directory on the filesystem as a pack, not just ones in res://, and document it alongside .pck and .zip files as a fully supported third kind of pack. This would be my personal preference, but I'd like to hear some more opinions before I work on including this in the PR.

[scgm0]

I don't think there is any need to expose PackedSourceDirectory at the moment. Let this PR only fix bugs. 4 can be considered for completion in subsequent PRs.

[tracefree]

I don't think there is any need to expose PackedSourceDirectory at the moment. Let this PR only fix bugs. 4 can be considered for completion in subsequent PRs.

Makes sense. Would you say this PR is ready to be merged as is then, or should I include a check to prevent users from accessing the new internal functionality manually from GDScript until it's fleshed out and documented later? I'm thinking of adding something like if (p_pack == "res://") { return false } to _load_resource_pack.

[scgm0]

Reasonable, before the function is determined, user calls should indeed be prevented from producing unpredictable results.

[tracefree]

By doing some more tests I realized setting the argument replace_files to false does nothing when loading the first pack, even on the master branch. It is documented here: #89299. From looking at the source code it seems the setting only allows not overriding files of previously loaded packs, not the resource folder if it is on the filesystem instead of a main pack. The reason for it is the same as for the other bugs. I was able to fix this with the following changes to the PR: it now includes hidden files and directories (crucially, this includes the .godot/imported folder) and uses the correct value of p_replace_files when loading res:// instead of always setting it to false.

[tracefree]

As discussed earlier, I added a line to the documentation of load_resource_pack explaining the remaining limitation to DirAccess. The wording may be a bit awkward, I'd appreciate any feedback if the same thing can be said in a more straightforward way.

[tracefree]

I'm failing a unit test for the macOS editor. Unfortunately I don't have access to any Apple computers... does anyone have an idea for what could be causing this? I assume it might have something to do with how I'm iterating over hidden directories?

[AThousandShips]

That one happens randomly on macOS, we're still unsure why, I'll just run it again and it should fix itself

[scgm0]

How is the progress of this PR? Is it possible to merge in 4.3?

[tracefree]

How is the progress of this PR? Is it possible to merge in 4.3?

It's ready from my side. Since it's a bug-fix I assume it could even be cherrypicked for 4.2.x / 4.1.x?

[m4gr3d]

The logic looks good to me!

Worth calling out that the touched code is not specific to Android, so an additional review from #platforms and #core may be needed.

[tracefree]

I've been thinking, maybe PackedSourceFileSystem is a better name than PackedSourceDirectory?

Edit: Or PackedSourceVirtual, since the source isn't actually packed at all, but the name might be less clear.

[akien-mga]

Just a quick status update. This looks promising (and appears to solve a lot of important issues), but from a quick chat with @reduzio it seemed to us that there is a deeper design issue with the resource pack loading, which would need to be addressed properly instead of worked around like done here.

So this needs further research (likely from the @godotengine/core team), which may or may not lead to merging this solution as is, or a more involved one. Either case, since we're in feature freeze for 4.3 now and this is relatively sensitive code, we'll postpone this to 4.4 (and consider a 4.3 cherry-pick if the solution we end up merging seems suitable for it).

[tracefree]

Understood, thank you for the update! Please let me know if there's any way I can help.

[AThousandShips]

The other issue is off topic for this PR, but the 3.6 marked issue is now not linked to this so it can be tracked and discussed, not all fixes can be applied to 3.x because of compatibility and complexity

[tracefree]

For what it's worth, I think my fix would easily transfer to 3.x as well. I can look into it once we have an agreed upon solution for 4.x.

[ThatFinnDev]

I've found an issue with this fix (because it doesn't fix something crucial). When you load a resource pack with replace files enabled, it will only replace files if they haven't been referenced before loading the pck/zip.
For example, in the MRPResourcePack project, if the icon.svg is used in another scene as an image, the ProjectSettings.load_resource_pack function won't replace the icon.svg with replace files enabled.
I'm not talking about the image not updating in the other scene (even though that's broken too). When a file has been referenced before in any kind (with preload, in other scenes, in other resources) and you load a resource pack and then try to load the file with load("res://path-to-file"), you get the old one

[JHDev2006]

adding on to this, this fix is mostly what id need, however trying to replace files using load_resource_pack is hopeless. either checking which files, resources and scenes etc are loaded before the pck gets loaded, then unloading them. or just giving us a way to see what resources are already loaded would be incredibly helpful!

heck, resource loader has a mode which allows us to replace files upon loading a resource, it would be amazing if it could just reload existing resources, upon the pck file being loaded, thatd make mods SOOO much more flexible!

[tracefree]

Hi! It's been a while since I worked on this, but IIRC replacing files that have been previously loaded is not a trivial fix due to how the resource system works, and I think it would be out of scope for this specific PR. And as akien said earlier, the whole resource pack system has design flaws and will have to be fundamentally changed to properly fix all its various issues, so I am waiting on updates on those plans before I can build on this PR.

[mihe]

@tracefree Any chance you would be able to incorporate the feedback from @bruvzg above, within the next few weeks? If not, I could take this pull request across the finish line, if that's fine with you.

The issue that this PR solves has turned out to (perhaps unsurprisingly) affect the patch system introduced in #97118 as well, so it would seem to me that it's quite important that we get this fixed before 4.4-stable.

[tracefree]

@mihe Hi! Sorry, I got sidetracked and hadn't found the time to look into this issue once more even though I've been meaning to. Thanks for the reminder, I will look into it by this weekend at the latest!

[tracefree]

Some more thoughts in addition to my code review comment:

• I would be very happy for this bug to get fixed since there are so many associated issues. But I just want to point out once more that my PR still has a weird side effect: after adding a datapack and PackedSourceDirectory is activated, new files added to res:// will no longer be picked up by DirAccess::get_files_at etc since the way it works is that a "snapshot" of the current res:// directory is taken and interpreted as a PackedSource at the moment the first non-main datapack is loaded. It is a very niche problem since modifying res:// is not really intended during runtime, but it's also not forbidden and I can imagine that people will come up with scenarios e.g. with tool development where doing just that is necessary, but once a resource pack is loaded there will be unexpected behavior. This side effect is a consequence of my PR being a bandaid on top of the rigid way resource packs were originally implemented. Akien mentioned in an earlier comment that a more extensive solution tackling the underlying issues would be better. Has there been any progress on this front? Are there ongoing discussions? I'm of course happy for the PR to be merged as-is as a temporary solution if it doesn't make an eventual greater rewrite more difficult.
• There are some other PRs and proposals regarding unloading datapacks (Implement unload of PCK files #61286), mounting them to specific paths (Allow using ProjectSettings.load_resource_pack() into a subfolder godot-proposals#7769), adding custom prefixes (Add FileSystem & FIleSystemProtocol to allow custom path prefixes (custom://) #98544), etc. I hope my PR doesn't cause too much trouble for those efforts.
• Finally, if my PR gets merged, I think it would be neat to explicitly support loading filesystem directories as "PackedSources". It would only be a matter of removing a check preventing this and adding documentation. It would be cool e.g. for mod development, where one could more easily load the working directory of a mod instead of having to zip the folder every time after making a change before launching the game.

[Repiteo]

Thanks!