Skip to content

Conversation

@lilleyse
Copy link
Contributor

ImageBitmap was added in #7579 to reduce frame rate hitches when loading models and a follow up PR was added to fix a regression for images that were being flipped incorrectly #7701. The PR added a flag called preferImageBitmap and each system now had to opt-in to using ImageBitmap. While reviewing #9624 I noticed a pretty big omission where calls to fetchImage in Model.js were not setting this flag to true and were not taking advantage of ImageBitmap. Note that this was only happening for external images; embedded images were fine because they go through a different code path, loadImageFromTypedArray, which defaults to using ImageBitmap if available. Thankfully embedded images are much more common in 3D Tiles so the regression wasn't super serious.

This wasn't mentioned in the PR at all so I think it was a mistake rather than intentional. I looked at all the sample models and everything still looks correct after the change. I also tested the bike model in #7579 and saw the expected improvement.

I fixed this in GltfImageLoader too which will be used as part of the model rewrite.

@lilleyse lilleyse requested a review from sanjeetsuhag June 19, 2021 00:13
@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@sanjeetsuhag
Copy link
Contributor

I was able to confirm the reduction in frame rate hitches, all model loading Sandcastles look good and all tests pass. Thanks @lilleyse!

@sanjeetsuhag sanjeetsuhag merged commit 4e2ffc4 into master Jun 21, 2021
@sanjeetsuhag sanjeetsuhag deleted the model-image-bitmap branch June 21, 2021 21:03
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.

4 participants