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 imagery request render #6194

Merged
merged 15 commits into from
Feb 27, 2018
Merged

Fix imagery request render #6194

merged 15 commits into from
Feb 27, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Feb 7, 2018

Fixes #6193

invalidateAllTiles was not being called reliably at the correct time.

@ggetz ggetz requested a review from hpinkos February 7, 2018 19:37
@cesium-concierge
Copy link

Signed CLA is on file.

@ggetz, thanks for the pull request! Maintainers, we have a signed CLA from @ggetz, so you can review this at any time.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Feb 7, 2018

@ggetz I'm seeing a crash if I alternate between Enable terrain and Disable terrain in the example I pasted in #6193

DeveloperError: requestTileGeometry must not be called before the terrain provider is ready.
Error
    at new DeveloperError (http://localhost:8080/Source/Core/DeveloperError.js:43:19)
    at CesiumTerrainProvider.requestTileGeometry (http://localhost:8080/Source/Core/CesiumTerrainProvider.js:569:19)
    at doRequest (http://localhost:8080/Source/Scene/TileTerrain.js:149:48)
    at requestTileGeometry (http://localhost:8080/Source/Scene/TileTerrain.js:163:9)
    at TileTerrain.processLoadStateMachine (http://localhost:8080/Source/Scene/TileTerrain.js:97:13)
    at processTerrainStateMachine (http://localhost:8080/Source/Scene/GlobeSurfaceTile.js:384:20)
    at Function.GlobeSurfaceTile.processStateMachine (http://localhost:8080/Source/Scene/GlobeSurfaceTile.js:281:13)
    at GlobeSurfaceTileProvider.loadTile (http://localhost:8080/Source/Scene/GlobeSurfaceTileProvider.js:505:26)
    at processSinglePriorityLoadQueue (http://localhost:8080/Source/Scene/QuadtreePrimitive.js:768:26)
    at processTileLoadQueue (http://localhost:8080/Source/Scene/QuadtreePrimitive.js:759:9)

@hpinkos
Copy link
Contributor

hpinkos commented Feb 7, 2018

I'm also still seeing a problem where sometimes the tiles don't load in if I zoom in and double click the Disable terrain button.
Sometimes they start to load in when I move the camera, but not always

@mramato
Copy link
Contributor

mramato commented Feb 19, 2018

Just a reminder not to forget about this for next release, I've hit it several times today.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 20, 2018

@hpinkos I believe this is resolved. I moved the invalidate logic to right before we process the tile load queue. I am no longer seeing any errors when switching back and forth, and repeatedly clicking disable terrain always reload the globe.

@mramato
Copy link
Contributor

mramato commented Feb 20, 2018

@ggetz, not sure if this is the same bug but I still see this in this branch:

  1. Start Cesium Viewer: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/fix-imagery-requestRender/Build/Apps/CesiumViewer/index.html
  2. Switch to Ellipsoid via Base Layer Picker (nothing will update until you move the mouse)

@ggetz
Copy link
Contributor Author

ggetz commented Feb 20, 2018

@mramato That's a regression because of the changes here. Fixed and added a new spec. @hpinkos in your sample code, you should no longer need to request a render after setting the terrain provider.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 21, 2018

@ggetz I'm still seeing issues here. If I double click the Disable terrain button, I can still get a globe with no tiles. It doesn't happen as consistently, but I can still reproduce it within a few tries.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 21, 2018

@hpinkos I don't think I can reproduce.

The first time I hit disable terrain, it takes a second or two for the new terrain provider to load in, but will re-render without mouse input. That's the same behavior as when we are not in requestRender mode.

If you comment out
viewer.terrainProvider = cesiumTerrainProviderMeshes; in your example, disbaleTerrain doesn't cause that initial delay since it was initially loaded with the EllipsoidTerrainProvider

@hpinkos
Copy link
Contributor

hpinkos commented Feb 21, 2018

@ggetz I can show you when you're in the office

@hpinkos
Copy link
Contributor

hpinkos commented Feb 26, 2018

@ggetz I just merged master and pushed it to your branch

@hpinkos
Copy link
Contributor

hpinkos commented Feb 26, 2018

Looks like this fixed the issue! Thanks @ggetz

@bagnell can you please review the Globe and QuadtreePrimitive changes?

@bagnell
Copy link
Contributor

bagnell commented Feb 27, 2018

@hpinkos @ggetz This looks good to me.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 27, 2018

Thanks @bagnell! @hpinkos tests are cleaned up as well now.

@hpinkos
Copy link
Contributor

hpinkos commented Feb 27, 2018

Thanks @ggetz!

@hpinkos hpinkos merged commit 57061f0 into master Feb 27, 2018
@hpinkos hpinkos deleted the fix-imagery-requestRender branch February 27, 2018 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants