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

Discourage access of res:// with Directory. #2280

Closed
TheDuriel opened this issue Mar 22, 2019 · 6 comments · Fixed by godotengine/godot#47413
Closed

Discourage access of res:// with Directory. #2280

TheDuriel opened this issue Mar 22, 2019 · 6 comments · Fixed by godotengine/godot#47413
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository discussion

Comments

@TheDuriel
Copy link
Contributor

TheDuriel commented Mar 22, 2019

This has been a reoccurring issue for new users; trying to access the PCK/res:// using Directory, only to find cases of unexpected (in their eyes) behavior.

The biggest example of this is "missing files", iv'e encountered several users trying to load resource files, typically audio, through directory. Mainly to dynamically assemble an array of file paths for sfx to randomly choose from. Obviously this does not work, as the audio file is not present on export. (.import files can be parsed as a workaround.)

I think it would be a good idea to discourage the use of Directory to access the PCK/res://. There is no scenario in which using it is "better" than using static file paths. The above usage example is more cleanly implemented by keeping an array of file paths in a class. And randomly picking a file from there. This also allows preloading for better performance.

Sidenote: On windows there are additional reasons against accessing PCK/res:// with directory. But those are bugs, and i'll probably make issues about those once i can replicate them.

@nobuyukinyuu
Copy link

I explicitly used Directory just recently in a project to enumerate files in a resource folder for numbering (maps, for guessing a level number if it's not explicitly stated elsewhere). The only reason I went with using Directory was because I couldn't easily find any method to use that effectively lists valid resource paths. For smaller lists of resources, such as in the random audio stream example you mentioned, I did however go with a constant array of preloads, but there are some situations where a user may want to explicitly access a resource folder to enumerate it, and Directory does seem like a really dirty way to do it. Perhaps there's a better way?

@bojidar-bg
Copy link
Contributor

I think it would be better to fix the Directory class when working with .pck files (godotengine/godot#25672), instead of just documenting the bug.

@TheDuriel
Copy link
Contributor Author

There is no bug here. Directory is working as intended (aside from the separate host windows issues.)

It can't find the files because they don't exist. Users expect them to exist however. They are wrong to do so, and should be informed about this.

If directory is meant to work on the PCK is a separate discussion. I'd say its incidental. It happened to work, and in my opinion should not.

@bojidar-bg
Copy link
Contributor

@TheDuriel .PCK files are handled differently than normal filesystems, see the class named FileAccessPacked or something like that. If you can load() or open a file, there is probably a way to list it. If the proper way is not via the Directory class, then that should be documented, and maybe even the Directory class be made to throw errors when opening res:// files. Otherwise, the Directory or FileAccessPacked class should be fixed.

@TheDuriel
Copy link
Contributor Author

Lets say i have a Sound.ogg file in res://.

I open res:// with directory.

In the editor: I see the physical file sitting on my drive.

When exported: I don't see it. The file did not get exported, and Godot remapped the path used by load() to the actual imported file.

This is the correct behavior. And what users will currently encounter.

They will still be able to see .import files, from which they may extrapolate the remapped file path. This is a workaround.

What i propose for the docs, is to explain this behavior. And to point out that if you need a list of file paths to load from, manually keep that list in a script file somewhere. Scanning directories in the PCK, to extrapolate file paths to load from is a slow and high effort solution prone to mistakes and errors.

When it comes to actual engine functionality, throwing an error when trying to open res:// / a .pck with directory sounds like a good move. It gets the point across. And we can point to better solutions. (For example the fact that resource packs should have an access point script listing the paths. We should probably add docs about resource packs for updates and dlc at some point in the future anyways.)

@MuffinManKen
Copy link

If the Directory class was fixed to return data as the user expected, in what way would scanning directories be "prone to mistakes and errors"?

Having a directory of images/sounds/etc that are loaded on startup automatically can lead to fewer errors. Having to manually update a script file when you add an asset like that seems ridiculous to me.

If I can load() res://test.png with it automagically fixing things to make it look like that file is there, then other API functions should do the same; Directory should list that file as existing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:class reference Issues and PRs about the class reference, which should be addressed on the Godot engine repository discussion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants