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

Improvement to terrain/imagery tile load ordering #4613

Closed
wants to merge 8 commits into from

Conversation

kring
Copy link
Member

@kring kring commented Nov 6, 2016

This change groups tiles that need loading into three tiers:

  • high - Tiles that are blocking refinement (their parents don't meet SSE, but we can't refine because these tiles are not renderable yet)
  • medium - Tiles that were selected for rendering this frame.
  • low - All other tiles, such as parent tiles we refined and extra tiles needed to complete quads.

Within each tier, tiles are loaded in traversal order, which currently is not ideal but improving that will be a separate PR.

It's surprisingly hard to benchmark reliably, but performance seems snappier, and if nothing else this strategy is a lot more justifiable than the "just load in traversal order" that we were doing before.

In TerriaJS and probably lots of other applications, it's common to have the camera in a particular place and everything loaded, and then enable another imagery layer. This PR makes that scenario noticeably quicker because it strictly loads the new layer on all the visible tiles first, before loading any extraneous nonsense. You can easily try this out in Viewer by switching the base map after the view is loaded.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

This PR makes that scenario noticeably quicker because it strictly loads the new layer on all the visible tiles first, before loading any extraneous nonsense

Any chance you could make an annotated gif or video of this for outreach purposes?

// and because we (currently) must have all children in a quad renderable before we
// can refine, this pretty much means tiles we'd like to refine to, regardless of
// visibility. (high)
// 2. Tiles that we're rendering. (medium)
Copy link
Contributor

Choose a reason for hiding this comment

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

It might just be terminology, but, for my understanding, why are "tiles that we're rendering" in the load order at all? Aren't they already loaded?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can render a tile before it's fully loaded. For example, if we add an imagery layer, terrain tiles don't disappear or unrefine while we're loading the new layer. Also upsampling (for better or worse) means that we can render terrain tiles before the terrain data is completely loaded.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gotcha, so it's tiles that we're rendering and are pending an update.

var i = 0;
var tile;

for (i = tileLoadQueueHigh.length - 1; i >= 0; --i) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of having three loops here, can we just move this to a utility function and pass in the queue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I did that in #4616.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Is it possible to add any other reasonable unit tests to verify that the right tiles are in the right queue?

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

@lilleyse can you also review this? It is potentially relevant to 3D Tiles.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 7, 2016

Thanks @kring, just those comments.

@lilleyse
Copy link
Contributor

These changes here and in #4616 look good. I don't much else to add but to confirm that everything does feel snappier.

@pjcozzi
Copy link
Contributor

pjcozzi commented Nov 10, 2016

Thanks @lilleyse.

@kring is there anything else you want to do before we merge (e.g., the Sandcastle example)?

@kring
Copy link
Member Author

kring commented Nov 11, 2016

I'm closing this because #4616 contains all of these changes plus some improvements, and if you actually merge this first, as I originally suggested, you'll probably get merge conflicts.

@kring kring closed this Nov 11, 2016
@kring kring deleted the betterLoadOrdering branch November 11, 2016 00:22
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.

3 participants