File format fix #160
File format fix #160
Conversation
…handle query strings for image URLs.
…tely and after the ends with comparison since ends with is probably more common.
Runtime/Scripts/GltFast.cs
Outdated
| || path.EndsWith(".jpeg",StringComparison.OrdinalIgnoreCase)) return ImageFormat.Jpeg; | ||
| if(path.EndsWith(".ktx",StringComparison.OrdinalIgnoreCase) | ||
| || path.EndsWith(".ktx2",StringComparison.OrdinalIgnoreCase)) return ImageFormat.KTX; | ||
| if(path.IndexOf(".png", StringComparison.OrdinalIgnoreCase) >= 0) return ImageFormat.PNG; |
There was a problem hiding this comment.
This is somewhat redundant to the conditions above and has a potential of creating false positives.
A better approach would be to search for the last occurance of ., assume it separates the extension and look at the 3-4 following chars.
Have a look at UriHelper.IsGltfBinary for reference. It does the same for .gltf/.glb
There was a problem hiding this comment.
I thought of something like that but query strings can contain periods so that would still not resolve the issue.
The redundancy is another matter and will fix. Was a perhaps misguided attempt to have some holistic performance, since the majority of gltfs won't have query strings.
There was a problem hiding this comment.
Since this is a specific fix to handle URL queries we could look from first "?" and forward to "." to find the file ending and compare that if you are worried about false positives for filenames or query strings with .png in them.
…tart and then look for the file format before that. If no query string exsist we simply look at the end of the fpath.
|
This looks good. Usually I should have asked you in the first response to provide some unit test for this method (to make sure we don't break anything and don't miss corner cases), but I looked at it and decided this also needs to be restructured/refactored outside of the Thank you very much! |
|
fyi: unit tests would've been useful indeed: I wrote some and revealed cases that led to crashes! See my commit to see what I changed. Anyways, should be fine now. thanks |
If a GLTF had URL query strings as part of its image URLs it GLTFast.cs would not recognize the file format. This was causing problems for me since I was getting GLTFs from a CDN that auto-generated the GLTFs with things like texture version info in the query strings. I fixed this by adding a set of "contains" checks after the "ends with" checks in GLTFast.cs. I added them at the end so that they will only be used if it does not end with a file format since I suspect that this will still be far more common.