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

Fix stripping of slashes from paths #486

Merged
merged 1 commit into from
May 23, 2024
Merged

Fix stripping of slashes from paths #486

merged 1 commit into from
May 23, 2024

Conversation

pmcgvr
Copy link
Contributor

@pmcgvr pmcgvr commented May 17, 2024

Context

Our team ran into issues where slashes were being stripped when a filepath with a protocol was supplied. For example, say duck.gltf references ducktex.png. If we supplied a URI like file://duck.gltf into tinygltf it would search for file:/ducktex.png (note the single slash).

This is because GetBaseDir() strips out the final slash. In most cases this is fine because JoinPath() adds the "/" back in. However, JoinPath() has the following check:

    if (lastChar != '/') {
      return path0 + std::string("/") + path1;
    } else {
      return path0 + path1;
    }

This means that for the "file://duck.gltf" case, GetBaseDir("file://duck.gltf") = "file:/". And then JoinPath("file:/", "ducktext.png") = "file:/ducktext.png".

The simple solution in this PR is to just leave the slash in when getting the base directory. I'm happy to modify JoinPath as well if you would like (I think we can remove the special if logic but wanted to keep this diff as simple as possible).

Testing

Ran tests using the test_runner:

tinygltf % python3 test_runner.py
Testing: /Users/pmcg/github/glTF-Sample-Models/2.0/AnimatedMorphCube/glTF/AnimatedMorphCube.gltf
Testing: /Users/pmcg/github/glTF-Sample-Models/2.0/AnimatedMorphCube/glTF-Binary/AnimatedMorphCube.glb
Testing: /Users/pmcg/github/glTF-Sample-Models/2.0/DamagedHelmet/glTF/DamagedHelmet.gltf
...
Success : 213
Failed  : 0

@syoyo
Copy link
Owner

syoyo commented May 17, 2024

Good find! GetBaseDir does not consider URI path(starts with 'proto://').

I think your PR should work well, but let me give some time to review the PR.

@syoyo
Copy link
Owner

syoyo commented May 20, 2024

After some thinking, I think it'd be better to consider proto:// and //(UNC path. \\ on Windows https://www.techtarget.com/whatis/definition/Universal-Naming-Convention-UNC ) in GetBaseDir and JoinPath.
For example,

GetBaseDir: path
  if path.contains("://"): Split into ["***://", ***]
  else: Split at last '/'(`\`)

And JoinPath considers both //(\\) and / case

@pmcgvr
Copy link
Contributor Author

pmcgvr commented May 21, 2024

@syoyo got it, I think the issue with this is that proto:// paths can also have their own directory structure. For example: file://mydir/myimage.png

The proposed pseudocode above would not handle this case, so then we would need a case which handles where proto:// is the base dir or if proto://.../... is the directory structure. In general it feels like this would lead down a rabbithole of special-casing. I am happy to make the changes but just wanted to propose the option I have right now where we basically allow the user to supply in whatever file structure they would like.

If we are non-destructive to the pathing (don't remove any \ or / at all) then it seems like this could safely work on any system or path. I found some examples where other open source repos appear to take the same approach:

Let me know your thoughts, I'm happy to special case proto:// and modify the JoinPath logic if you still prefer that option.

@syoyo
Copy link
Owner

syoyo commented May 21, 2024

@pmcgvr super awesome! thanks a lot! oh code fragments in llama.cpp(I love llama.cpp).
Thus your initial proposal should work well. PR will be soon merged, but let me give few more time to understand how other OSS deal with it.

@syoyo syoyo merged commit f03fe26 into syoyo:release May 23, 2024
8 checks passed
@syoyo
Copy link
Owner

syoyo commented May 23, 2024

Merged! Thanks!

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