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 VRTheWorld terrain geometry with ImageBitmap #7699

Merged
merged 1 commit into from
Apr 1, 2019
Merged

Fix VRTheWorld terrain geometry with ImageBitmap #7699

merged 1 commit into from
Apr 1, 2019

Conversation

OmarShehata
Copy link
Contributor

This fixes #7697. If you go to the terrain Sandcastle in master and try the VRTheWorldTerrain, you'll see incorrect terrain geometry.

It's because VRTheWorldTerrainProvider constructs geometry from a heightmap by reading the pixels! The fix is just to make sure flipY: false in the requestTileGeometry function.

@lilleyse

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ 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.

@lilleyse
Copy link
Contributor

lilleyse commented Apr 1, 2019

Thanks @OmarShehata.

@lilleyse lilleyse merged commit 57e6588 into CesiumGS:master Apr 1, 2019
@mramato
Copy link
Contributor

mramato commented Apr 1, 2019

If I'm reading this PR correctly, fetchImage always returns a flipped image by default, and this is new behavior with the last release? That sounds kind of bad to me and this fix is a band-aid on a bigger problem. Why would the generic Resource.fetchImage function return upside-down images by default?

@OmarShehata
Copy link
Contributor Author

OmarShehata commented Apr 1, 2019

@mramato I've gone back to our conversations in #7579 and it looks like I never explicitly mentioned this new default being true. It felt consistent with the rest of the codebase since any texture upload was flipped by default, but this does mean anyone who was using fetchImage() and doing anything with the pixels or using the image object outside of Cesium will now have it flipped with this release.

We can change the default to flipY: false, and then just make sure to flip all imagery in our codebase. The only problem is that if any user is using fetchImage() and then drawing that image in Cesium, it's going to be upside down by default, because ImageBitmaps cannot be flipped during texture upload. So we'd have to flip the images on materials too. This is in our control, but not if the user is fetching the image and then passing it themselves, but I don't know how common that is.

The original problem that complicated this is that textures are all flipped by default. If imagery was flipped in the shader instead this would be simpler. I remember we tried to do that but ran into issues getting it to work correctly with the projection.

I think this boils down to: is it more common to fetchImage to upload it as a texture, or to do something else? I agree that it makes sense for the generic fetchImage function to return an unflipped image by default. But we should be aware that this will result in flipped textures for users who pass a fetched image themselves, since ImageBitmaps cannot be flipped during texture upload.

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.

VRTheWorld terrain provider broken
4 participants