godot
godot copied to clipboard
godotengine
Fix "res://" being replaced by resource packs in the editor and on Android
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: #48563
- Bugsquad edit, fixes: #19815
- fixes #87274
- fixes #16798
- fixes #89299
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.
Similar problems also exist on Android, because on Android, "res://" is also a real local directory.
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").
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").
https://github.com/godotengine/godot/issues/87274 Although I didn’t mention Android in the introduction, I did initially discover this problem on Android.
#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.
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.
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.
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?
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:
- Do nothing. Then there would be undocumented / possibly undefined behavior of
load_resource_pack. - Leave it as is but document this relatively useless behavior.
- Somehow try to prevent users from doing this manually.
- 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.
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.
I don't think there is any need to expose
PackedSourceDirectoryat 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.
Reasonable, before the function is determined, user calls should indeed be prevented from producing unpredictable results.
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.
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.
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?
That one happens randomly on macOS, we're still unsure why, I'll just run it again and it should fix itself
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?
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.
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).
Understood, thank you for the update! Please let me know if there's any way I can help.
I hope don't forget about godot 3.6 all bugs reported for the 3.x branch are sent to 4.x ...
I hope don't forget about godot 3.6 all bugs reported for the 3.x branch are sent to 4.x ...
If there isn't a dedicated 3.x report for this please open one, the claim that all bugs are just sent to 4.x isn't true if they're specific to 3.x
@AThousandShips is an issue already reported for 3.x ... #54917
I say this because even I had reported a problem in 3.x #67953 and the solution was only for the 4.x and they closed the issue ...
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
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.
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
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!
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.