Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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

@ggetz ggetz force-pushed the fix-imagery-requestRender branch from 319c2ca to 428c3cf Compare February 26, 2018 21:22
@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
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants