Fix KTX/CRN Tiles #8064
Fix KTX/CRN Tiles #8064
Conversation
|
Thanks for the pull request @ProjectBarks!
Reviewers, don't forget to make sure that:
|
| return loadCRN(resource); | ||
| } else if (defined(imageryProvider.tileDiscardPolicy)) { | ||
| } else if (defined(imageryProvider) && defined(imageryProvider.tileDiscardPolicy)) { |
There was a problem hiding this comment.
Why was the imageryProvider.tileDiscardPolicy check added?
There was a problem hiding this comment.
I didn't add defined(imageryProvider.tileDiscardPolicy) I added defined(imageryProvider) so people can provide null imageryProviders.
Although, I am not sure this is the correct approach we should be using. We could:
- Set ready to true by default and then pass a reference of imageryProvider to
loadImage. - Move the loadImage occur in the requestImage function and set ready to true by default.
There was a problem hiding this comment.
Sorry, not enough caffeine yet. In the place you added the call for this, you have the ImageryProvider, so you can just pass it into this function instead of changing this function.
There was a problem hiding this comment.
Talked to @ProjectBarks offline. After looking into it a little more, I think this is the cleanest option for now (and it would have just crashed previously so it's not like we were guaranteeing any sort of behavior). Long term, we should probably unify ktx/crtn/resource image loading a bit more than it is now.
There was a problem hiding this comment.
Wrote up #8070, not a priority right now.
|
Looks like Material and Model may have the same issue with a regex happening on a resource instead of a url. Can you check it out and confirm one way or the other? |
|
@mramato Model looks fine but Material had instances where it could be a resource instead of a url. Made push to fix that. |
|
Thanks @ProjectBarks that was my assumption as well. |
The
loadImagefunction would test against the Resource file and not the actual URL. This also fixes SingleImageTilesets to load KTX/CRN files as well.For testing try out this sandcastle. I was unable to find a working CRN file but if someone wants to test that before merging that would be fantastic!
In reference to: #7979