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

UfbxImporter: Add support for searching images #142

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

bqqbarbhg
Copy link
Contributor

@bqqbarbhg bqqbarbhg commented Sep 22, 2023

Some exporters write only absolute paths for files, yet it would be nice to resolve them.

For example if you export from Blender with the image residing in another drive on Windows, you get something like:

Texture: 455007087, "Texture::base_color_texture", "" {
    FileName: "W:\Data\ExternalTextures\texture.png"
    RelativeFilename: "W:\Data\ExternalTextures\texture.png"
    ...
}

Some other importers seem to scan all sibling subdirectories to the FBX file recursively(!) and match only based on filename. This feels a little excessive so I propose attempting to load N parent directories of the file, configurable via the imageSearchDepth option.

The importer would try to search the following paths in order depending on the imageSearchDepth option:

Depth Path, relative to FBX parent directory
0 W:/Data/ExternalTextures/texture.png
1 ./texture.png
2 ./ExternalTextures/texture.png
3 ./Data/ExternalTextures/texture.png

Using imageSearchDepth=-1 allows for unlimited amount of subdirectories to be searched.

Note that the actual relative path is always preferred, if it exists, so for a file like:

Texture: 455007087, "Texture::base_color_texture", "" {
    FileName: "W:\Data\Textures\texture.png"
    RelativeFilename: "Data\Textures\texture.png"
    ...
}

The following paths would be scanned in order:

Depth Path, relative to FBX parent directory
0 ./Data/Textures/texture.png
1 ./texture.png
2 ./Textures/texture.png
3 ./Data/Textures/texture.png

Design considerations

  • Should image searching be enabled by default? (if so, only sibling files or deeper)
  • Should we even try to load absolute paths, or should there be a setting to forbid it?
  • If we forbid absolute paths, should we forbid relative parent paths as well? So users can't use something like RelativeFilename: ../../../etc/passwd, however having a directory structure like ./models/model.fbx ./textures/texture.png would make RelativeFilename: ../textures/texture.png a valid use case.

@mosra
Copy link
Owner

mosra commented Sep 22, 2023

Hi, thank you!

I vaguely remember we discussed something related to this when you did the original PR. Don't remember what the conclusion was back then, but as the situation is now I guess it's not practical to either (a) expect that FBX files either have reasonable paths or (b) expect each application code to have similar logic to find the lost files via file loading callbacks, so something needs to be done on this front in any case :D

Are you aware of similar nastiness happening for OBJ, or is it really just FBX? For example I wouldn't expect glTF files to be this bad, but about OBJs I'm not sure. Hm, that's probably what I was originally wondering about, if there would be a general use for such a "got a crap model, find my files" feature, or if it's really just needed for a single file format. Like, instead of having it localized in this plugin, providing some generalized plugin-independent API (which this plugin would be using implicitly, and others could opt in to it).

Looking at the code, I'm not sure what the interactions with file callbacks should be. Right now it seems to be calling Path::exists() even with file callbacks set, which doesn't really make sense. It should probably not try to do any filesystem queries in that case and just assume file callbacks do that instead (i.e., behave same as if imageSearchDepth was 0).

Apart from images, are there any other externally referenced data that could possibly suffer from this? Baked caches and such?

I think I'll take some time to think about the best place to put this. Let me know if there's someone blocked by this so I can prioritize appropriately.

(Also sorry for the current trashy state of the CI, I just pushed various fixes to master to resolve the errors.)

@bqqbarbhg
Copy link
Contributor Author

Thanks for the comment, good points! It's definitely not an FBX-exclusive problem, and it could appear in .obj files as well. If using ufbx, this PR would fix it for them, but I agree it would be better to have consistent behavior between multiple importers.

For (a), definitely not, it's a reasonably common practice to distribute FBX files with their textures loosely on the side. Especially with Blender as you need to be really careful with your export options to avoid that happening.

I believe (b) is really an interesting consideration, my first thought was to implement this in application code, but ended up thinking it would be generally useful here (Magnum really blurs the lines between engine and middleware, so it's hard to say where the responsibilities end :D). One one hand it would be good for users to write their own search strategies, as this does not have a singular obvious solution. On the other hand however files with wonky textures (especially FBX) are relatively common so it would be nice to handle it for users. Maybe one option would be to expose a callback for resolving filenames without the users having to handle file IO (extra points if we'd provide one or two pre-made strategy callbacks), though I'm not sure how much better that is.

And yeah file callbacks, I didn't really consider them too much yet.. the reason I opted to use Path::exists() instead of trying to call openFile() was to avoid error spam from loading non-existent files, and if plugins don't handle missing files very gracefully.

FBX files can contain geometry caches, which I assume might have similar issues in the wild. However that feature is very rarely used and not currently supported in the Magnum loader. However, if you decide to add a generic file search API it could be used for that purpose as well.

Personally, I'm not blocked by this, I will ask around and if it turns out to be an urgent issue I will work around it with the existing file callbacks so no hurry!

No worries about the CI, I'll rebase the branch and it'll look like it never happened :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants