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

Ignore empty tile 's SSE when refining tile #9140

Closed
wants to merge 12 commits into from
Closed

Conversation

baothientran
Copy link
Contributor

We got a support issue where the tileset has some of the empty tiles that are near the leaf of the tree. These tiles usually have low SSE, which prevents the current tile from refining further. It is unfortunate that we have to ignore empty tile's SSE and traverse down the tree until we meet a tile that has content available to render.

@lilleyse Can you please take a look at it? Please let me know if I should 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.

Reviewers, don't forget to make sure that:

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

@baothientran
Copy link
Contributor Author

baothientran commented Sep 6, 2020

Since we ignore the empty tile, I notice there is a different behavior in the tileset that has mix add-replacement refinement. This is the sandcastle example.

Below is the tree of the tileset

                C(Replace)
     E(ADD)                    E(ADD)
C                C            C               C

In the master branch, when we zoom in, the content leaf tile is iteratively render. But in this branch, they are rendered all at once

Master branch:
Screenshot_20200906_112816

This PR:
Screenshot_20200906_112630

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

3 similar comments
@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

1 similar comment
@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@cesium-concierge
Copy link

Thanks again for your contribution @baothientran!

No one has commented on this pull request in 90 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 90 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

@lilleyse
Copy link
Contributor

I think we should close this and instead focus on addressing #9356

There are three different ways that we're refining empty tiles

  • Don't treat empty tiles as special. If empty tiles exist they may be selected and appear as holes. (Current Cesium Native behavior, and most spec compliant)
  • Skip empty tiles during refinement that meet the SSE. (Current CesiumJS behavior)
  • Skip empty tiles during refinement regardless of whether they meet the SSE. (This PR)

@lilleyse lilleyse closed this Dec 14, 2021
@lilleyse lilleyse deleted the empty-tile-zoom branch December 14, 2021 16:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants