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

Provide a reliable way to see original resources in a directory #96590

Merged
merged 1 commit into from
Nov 11, 2024

Conversation

reduz
Copy link
Member

@reduz reduz commented Sep 5, 2024

When exporting a project, resources are often moved, converted or remapped (source import files are gone, text scenes are converted to binary, etc).

This new function is a convenience that allows to list a directory and obtain the actual original resource files.

Example

var resources = ResourceLoader.list_directory("res://images")
print(resources)

Result will be something like:

["image1.png","image2.png","image3.png"]

instead of

["image1.png.import","image2.png.import","image3.png.import"]

@reduz reduz requested review from a team as code owners September 5, 2024 06:56
@AdriaandeJongh
Copy link
Contributor

AdriaandeJongh commented Sep 5, 2024

If you'll allow me to scrutinize the function name: if the function essentially lists files and not directories, perhaps an alternative name could be:

  • list_directory_files
  • list_files_in_dir
  • get_files_in_directory (with get_* being perhaps more in line with other function names in ResourceLoader)
  • get_files

@reduz
Copy link
Member Author

reduz commented Sep 5, 2024

It lists both files and directories, it just appends a "/" to the end of the directory, I forgot to make note of this, will do so.

@JHDev2006
Copy link

Would this also mean that we wouldn't have to deal with .remap in exported games? Looks great! Looking forward to seeing this improvement!

@KoBeWi
Copy link
Member

KoBeWi commented Sep 5, 2024

So I compared editor results with exported results and got this:
Editor get_files_at()

["Scene.tscn", "Script.gd", "icon.svg", "icon.svg.import", "textfile.txt"]

Editor list_directory()

["../", "./", "Folder/", "Scene.tscn", "Script.gd", "icon.svg"]

Exported get_files_at()

["Scene.tscn.remap", "Script.gd.remap", "Script.gdc", "icon.svg.import"]

Exported list_directory()

["Folder/", "Scene.tscn", "Script.gd", "Script.gdc", "icon.svg"]

Notes:

  • Editor lists . and .. for some reason. This should be removed.
  • list_directory() does not include non-resource files. This should be stated in the documentation.

Other than that seems to work correctly.

core/io/resource_loader.cpp Outdated Show resolved Hide resolved
core/io/resource_loader.cpp Outdated Show resolved Hide resolved
core/io/resource_loader.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member

Calinou commented Sep 6, 2024

Does this PR fully supersede #59334?

@KoBeWi
Copy link
Member

KoBeWi commented Sep 6, 2024

Well my PR lists resource files together with non-resource files and excludes folders. But that's just alternative implementation, it's fully superseded.

When exporting a project, resources are often moved, converted or
remapped (source import files are gone, text scenes are converted to binary,
etc).

This new function allows to list a directory and obtain the actual original
resource files.

Example

```GDScript
var resources = ResourceLoader.list_directory("res://images")
print(resources)
```
Result will be something like:
```
["image1.png","image2.png","image3.png"]
```
instead of
```
["image1.png.import","image2.png.import","image3.png.import"]
```
@reduz
Copy link
Member Author

reduz commented Oct 7, 2024

Applied changes suggested.

while (!d.is_empty()) {
bool recognized = false;
if (dir->current_is_dir()) {
if (d != "." && d != "..") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's weird that this is needed. DirAccess' include_navigational is false by default. The fact that it's different between editor and export means there is some bug.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have that feeling too, but I ended up adding it regardless.

Copy link
Member

@KoBeWi KoBeWi Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried listing directory using list_dir_begin() in GDScript and it's correctly skipping navigational paths. Seems like open() in C++ works a bit differently (GDScript uses _open()), but not sure what causes the wrong behavior.

EDIT:
Nevermind, it's because get_next method is bound to _get_next(), which does the skipping. Doesn't explain why it's different after exporting though.

EDIT2:
Ok this might be caused by DirAccessPack.
In any case, skipping it manually seems to be the solution anyway.

@nonchip
Copy link

nonchip commented Oct 8, 2024

please refer to comments in the PR this supersedes why it's not actually "reliable".

for example mine there and there

tldr: you're guessing that a certain filename means that it used to have a different one, but the mere existence of ExportPlugins makes that assumption questionable at best. like KoBeWi said in the other PR, there's no way of actually doing this "reliable" without creating a list during export, and a simple s/.remap// is not enough to resolve that list. this PR is just copying a common gdscript hack/workaround to core instead. it'll work often/usually, but doesn't do what it claims.

@reduz
Copy link
Member Author

reduz commented Oct 8, 2024

@nonchip Yes, but that is a very corner case and generally external to the core engine, and even with export plugins remaps are still used, so it should work in almost every case. A "proper" solution is likely so complex that its not worth it.

Likewise, this PR is fine as-is, and even if a better solution is deemed worth to implement in the future, the API exposed to the user is not changed.

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Nov 11, 2024
@Repiteo Repiteo merged commit c8ff788 into godotengine:master Nov 11, 2024
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 11, 2024

Thanks!

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.

Listing directories in exported project returns only *.import files
9 participants