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

Don't reproject Web Mercator imagery tiles unnecessarily #4339

Merged
merged 30 commits into from
Oct 19, 2016
Merged

Conversation

kring
Copy link
Member

@kring kring commented Sep 19, 2016

Previously, Cesium would reproject all Web Mercator imagery tiles to Geographic on load. This had two costs:

  1. It took some time (not much, probably).
  2. It introduced some blurring in the imagery because destination pixels were not aligned with source texels.

And then, of course, Cesium would reproject those Geographic images onto the globe with the normal 3D perspective projection. So effectively, Cesium reprojected imagery tiles twice.

After this PR, Cesium includes Web Mercator texture coordinates with the terrain tiles, so Web Mercator tiles can be projected directly onto the screen without going via Geographic in most cases. Some notes:

  • The imagery looks a bit sharper after this change.
  • I can't think of any reason that interpolating texture coordinates linearly in Web Mercator space is any less accurate than interpolating in Geographic space. In fact I think interpolating in Web Mercator should be a bit better because it's conformal-ish.
  • Terrain meshes are now bigger; there's a an extra 12-bit compressed float per vertex, which currently means an extra whole float per vertex. I spent way more time than I should have trying to avoid this. I thought I could eliminate the height attribute in most cases, especially when using 3D only. And I wrote some really hairy code to support this (in the terrain3 branch if you want to gaze upon the horror). But in the end it was just too hard and I gave up.
  • The Web Mercator coordinates are currently only used for terrain tiles that fall entirely within the Web Mercator bounds (+/- ~85 degrees latitude). For tiles that cross that line, we still reproject to geographic. We could avoid this by making sure that our terrain meshes always include vertices at +/- 85 degrees latitude (i.e. add them when we create the mesh) so a triangle never crosses that line. Triangles that cross the Web Mercator boundary cause problems when interpolating texture coordinates for fragments.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 20, 2016

The imagery looks a bit sharper after this change.

Do you have a nice before/after (maybe with an image diff if useful) we could use for twitter and the release blog?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 20, 2016

Terrain meshes are now bigger; there's a an extra 12-bit compressed float per vertex, which currently means an extra whole float per vertex...

Ouch. @bagnell and I will brainstorm since the trend is to use less memory, not more. Maybe it is unavoidable. Maybe we can optimize something else. Maybe this is the best we can do.

@@ -3,6 +3,7 @@ Change Log

### 1.26 - 2016-10-03

* Removed an unnecessary reprojection of Web Mercator imagery tiles to the Geographic projection on load. This should improve both visual quality and performance slightly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure we can make the performance claim in practice? In the steady state, this is probably worse.

* @param {Rectangle} [result] The object onto which to store the result.
* @returns {Rectangle|undefined} The modified result parameter, a new Rectangle instance if none was provided or undefined if there is no intersection.
*/
Rectangle.simpleIntersection = function(rectangle, otherRectangle, result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Add to CHANGES.md please.


stride = (numCompressed0 + numCompressed1) * sizeInBytes;

return [
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you run this through the formatter?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we're using the same editor, so running it through "the formatter" probably doesn't make sense. But I moved the open curly to the same line as the open square bracket and stuff.

@@ -156,7 +156,7 @@ defineSuite([
});
}

it('reprojects web mercator images', function() {
it('reprojects web mercator images when necessary', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are any other new unit tests needed, e.g., request completely inside +/- ~85 latitude and completely outside?

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 20, 2016

The Web Mercator coordinates are currently only used for terrain tiles that fall entirely within the Web Mercator bounds (+/- ~85 degrees latitude). For tiles that cross that line, we still reproject to geographic. We could avoid this by making sure that our terrain meshes always include vertices at +/- 85 degrees latitude (i.e. add them when we create the mesh) so a triangle never crosses that line. Triangles that cross the Web Mercator boundary cause problems when interpolating texture coordinates for fragments.

This could be done at runtime with quick reject tests, right? Do you think that would be cleaner than the current approach, which is...complicated. Or are you suggesting a breaking terrain spec change and doing this offline? The later is not out of the question, it would just have to be longer-term.

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 20, 2016

@bagnell can you please also review this when you are available?

@kring
Copy link
Member Author

kring commented Sep 21, 2016

Ouch. @bagnell and I will brainstorm since the trend is to use less memory, not more. Maybe it is unavoidable. Maybe we can optimize something else. Maybe this is the best we can do.

Here are some ideas:

  • Include the vertical geographic and/or web mercator texture coordinate, but not both unless we actually have two different imagery layers with two different projections. Aside from the complexity of managing optional vertex attributes like this, we also need to worry about other things that use the texture coordinates, like the water effect.
  • Eliminate the height. It's used for upsampling, and it's used in 2D and CV. We could handle the upsampling case by storing the heights separately (they don't need to be sent to the GPU). And of course 2D and CV don't matter if you're using the 3D-only option. But this is a lot of complexity and most applications won't benefit because they don't disable 2D and CV. We could at least let every app get some benefit by creating meshes for 2D and CV on the CPU (when displaying those views) rather than using vertex shader displacement.

@kring
Copy link
Member Author

kring commented Sep 21, 2016

We could at least let every app get some benefit by creating meshes for 2D and CV on the CPU (when displaying those views) rather than using vertex shader displacement.

Or maybe by using a separate buffer for the heights when rendering in this view.

@kring
Copy link
Member Author

kring commented Sep 21, 2016

This could be done at runtime with quick reject tests, right? Do you think that would be cleaner than the current approach, which is...complicated. Or are you suggesting a breaking terrain spec change and doing this offline? The later is not out of the question, it would just have to be longer-term.

I think it can be reasonably done at runtime. For heightmap terrain it's easy, for quantized mesh it's a bit of work, but not too hard. I'm not sure the overall complexity would go down here though. ;)

@kring
Copy link
Member Author

kring commented Sep 21, 2016

Before:
image

After:
image

Diff (used https://huddle.github.io/Resemble.js/):
image

It's most obvious in contrasty bits, so the labels mostly in these screenshots. Save the two images and flip back and forth between them to really see it. On contrastier maps (like Bing Maps Roads for example), lots of map features look noticeably sharper. Overall, it's admittedly fairly subtle, though.

@kring
Copy link
Member Author

kring commented Sep 22, 2016

Ok I think this is ready. Of course, it's up to you guys if you think the improvement is worth the extra float. I tried and failed to measure any performance or memory usage difference with this change. Actually, the memory usage of the GPU process in Chrome was consistently 10 meg (out of 500+!) lower after this change, possibly just because there's less texture creation now and perhaps Chrome doesn't clean up old ones very aggressively?

I'd be interested in the options I mentioned above for eliminating the height or the two sets of texture coordinates in order to reduce the amount of vertex data, but it's not straightforward and I think it's more important to do some other things first, like improve the tile load process.

@denverpierce
Copy link
Contributor

Gif of both versions:

animation

@denverpierce
Copy link
Contributor

I modified the Path example to fly over the continental U.S. with the camera pointed straight down to test performance. I ran MSI Afterburner in the background to watch results, keeping as many other things constant as I could (no other windows open, opening from an empty chrome, etc.).

VRAM usage was ~10mb lower on the new version, but RAM usage was ~100mb higher. GPU and CPU usage was about the same.

1.25:
image

noReproject:
image

@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 15, 2016

@bagnell could you please evaluate this at the bug bash?

@bagnell
Copy link
Contributor

bagnell commented Oct 19, 2016

This looks good to me. For the extra float, I'll add the ideas from @kring above to an issue.

@bagnell bagnell merged commit a8f0d0f into master Oct 19, 2016
@bagnell bagnell deleted the noReproject branch October 19, 2016 13:37
@pjcozzi
Copy link
Contributor

pjcozzi commented Oct 19, 2016

Thanks again @kring. I moved the CHANGES.md update to 1.27 in master.

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