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

Turn off throttle option when requesting for an availability tile from the parent terrain layer #9099

Merged
merged 4 commits into from
Aug 19, 2020

Conversation

baothientran
Copy link
Contributor

@baothientran baothientran commented Aug 18, 2020

The function checkLayer() in the CesiumTerrainProvider.js file is responsible for requesting an availability tile for the parent layer. However, the promise is delayed for a long time or never get resolved especially where the tile is overlapped with the child layer, which makes QuadtreePrimitive upsamples the tile from the parent tile. Turning off the throttle option in the Request object does solves the issue though. This is consistent with other requests coming from the other terrain code

This is the parent layer when all the tiles are available. Notice the three lines on the head of the two tiles at level 10
Screenshot_20200818_170231

This is the missing tiles from the parent layer when delay happens. Notice there are no three lines on the head of the tiles at level 10. Keeping zooming in doesn't solve the problem
Screenshot_20200818_170108

@lilleyse Can you please take a look at it? Please let me know if I need to add anything

@cesium-concierge
Copy link

Thanks for the pull request @baothientran!

  • ✔️ 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

@baothientran Definitely 👍 on the change. The terrain and imagery system used to throttle requests in the RequestScheduler but it clashed with 3D Tiles' priority system so we reverted it and let terrain prioritize/throttle according to its own rules. I think this code just never got reverted.

Can you make the same change for other terrain/imagery providers? A search for throttle: true brings up a few more results. Ultimately throttle: true should be removed everywhere except in Cesium3DTile, ResourceSpec, and RequestSchedulerSpec.

@baothientran baothientran changed the title Turn off throttle option when requesting for a availability from the parent terrain layer Turn off throttle option when requesting for an availability tile from the parent terrain layer Aug 18, 2020
@baothientran
Copy link
Contributor Author

@lilleyse I turned off the throttle options for all the terrain and imagery provider. Some tests are fixed along the way, but it shouldn't change the testing cases' purposes

@lilleyse
Copy link
Contributor

lilleyse commented Aug 18, 2020

@baothientran could you add the fix to CHANGES.md?

@lilleyse
Copy link
Contributor

@kring can you confirm that this is a good fix?

@lilleyse lilleyse requested a review from kring August 18, 2020 22:28
throttle: true,
throttleByServer: true,
});
promise = terrainProvider.requestTileGeometry(0, 0, 0, request);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it a problem that we're no longer calling createRequest here? So the request has no URL, for one thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it seems like createRequest() returns an empty request object with only throttleByServer option on. I kind of create the new Request for only the test case. Should I modify the createRequest() with throttle to be true for all of them?

Copy link
Member

@kring kring Aug 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ok, I hadn't realized createRequest also created a request without a URL. That makes sense, no need to change anything.

@kring kring merged commit 853ed28 into master Aug 19, 2020
@kring kring deleted the terrain-throttle branch August 19, 2020 02:15
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