Do not use a fixed tile grid for vector rendering #799

ahocevar · 2013-06-19T16:15:59Z

Instead, we create a new tile grid whenever renderFrame is
called, no animation is active, and the resolution is not in the
tile grid already. This gives better rendering results because
we get vector tiles at native resolutions.

ahocevar · 2013-06-20T16:39:53Z

This is ready for review now.

elemoine · 2013-06-20T18:43:07Z

src/ol/renderer/canvas/canvasvectorlayerrenderer.js

@@ -305,15 +312,32 @@ ol.renderer.canvas.VectorLayer.prototype.renderFrame =
      layer = this.getVectorLayer(),
      tileGrid = this.tileGrid_;

+  // lazy tile grid creation
+  if (!frameState.viewHints[ol.ViewHint.ANIMATING]) {


Quick comment before actual review: should the above be if not animating and not interacting? For example, I guess you don't want the cache to contain tiles at "while pinching" resolutions?

Good point. I'll change that. Thanks!

twpayne · 2013-06-21T14:48:42Z

All this in-place modification of the tile cache and messing about with the internals of a tilegrid is going to be slow and confusing.

I'm strongly opposed to this approach. Surely there is a better way,

elemoine · 2013-06-21T14:52:03Z

The "messing about the internals of a tilegrid" is mine :) Andreas currently creates a new tile grid each time a new resolution should be inserted.

I'm also not a big fan of the in-place modification of the tile cache, but I cannot see a better way.

twpayne · 2013-06-21T14:58:33Z

src/ol/renderer/canvas/canvasvectorlayerrenderer.js

+/**
+ * @define {number} The lowest supported resolution value.
+ */
+ol.renderer.canvas.MIN_RESOLUTION = 0.14929107086948487;


Where does this magic number come from?

Zoom level 21 in a default web mercator tiling scheme.

@ahocevar - would you mind adding a comment here with a link to #404?

ahocevar · 2013-06-21T14:59:08Z

The only other way I see (which I don't think would be faster or more elegant) is this:

The TileGrid fires an event when its resolutions change, with old and new resolutions as listener argument.
Each TileCoord is created with a reference to a TileGrid and registers a listener to the TileGrid's change event
The TileCoord's listener adjusts the z value accordingly.

twpayne · 2013-06-21T15:17:06Z

src/ol/renderer/canvas/canvasvectorlayerrenderer.js

+  if (!frameState.viewHints[ol.ViewHint.ANIMATING] &&
+      !frameState.viewHints[ol.ViewHint.INTERACTING]) {
+    // avoid rendering issues for very high zoom levels
+    var newResolution = Math.max(resolution, ol.renderer.canvas.MIN_RESOLUTION);


So does this mean that resolutions smaller than ol.renderer.canvas.MIN_RESOLUTION (Google zoom level 21) are not supported by the canvas vector layer renderer?

For now, we run into coordinate range issues for very long line segments, which makes them disappear. This restriction will go away once we introduce clipping before rendering geometries. See #404.

When zooming in beyond level 21, vector rendering is still available, but will be scaled from this MIN_RESOLUTION.

twpayne · 2013-06-21T15:40:53Z

I think that there are a number of other options, including:

Cache tiles by resolution, x, and y (instead of z, x, and y) - it might be possible to make this work
Don't use tiles for vector layers (my favorite)

The problem that this PR tries to address is just one aspect of a multitude of problems with the approach of rendering vector data into tiles and caching those tiles.

Using tiles for vector layers means that vector layers aren't really vector layers. They're really tile layers where the rendering of the tiles happens to be done on the client instead of the server. This means that you get all the scaling artefacts associated with tile layers, and you don't get the nice sharp rendering that vector layers should provide at all resolutions.

Using vector tiles has pathologically bad performance in several common circumstances:

Consider a vector with a single feature that is being edited. Even if the feature can span two, three or even four tiles if it happens to intersect the corner between tiles. Modifying the features causes four 512x512 tiles to be invalidated - that's four 512x512 canvases to re-render, even if the feature is only covers a few pixels! So, editing a 32x32 pixel feature can mean that you're really re-drawing 1024x1024 pixels. These pixels then have to be composited...
Compositing layers is slow, and the speed is directly proportional to the number of pixels that have to be composited. With a vector tile layer, a layer containing a single small feature is as expensive to compose as an entire raster layer. A huge amount of performance is wasted compositing transparent pixels. Due to the compositing of vector tile layers, OpenLayers 3 will only be able to support 3-4 vector layers, even if each vector layer only contains a single small feature. This is ridiculous.
These vector tiles will blur as the map rotates, since you are really rotating raster images instead of neatly rendering vector data.
You will have to invalidate your cache of vector tiles under several common circumstances, for example:
- A feature changes (both the tiles where the feature was, and the tiles where the feature is now have to be re-rendered)
- The map rotates and but you want labels to not rotate with the map.

In short, I think that this approach of rendering vector data into raster tiles is fundamentally flawed in terms of performance, image quality and scalability and it completely defeats the point of using vector data in the first place.

I think we should find a better approach.

ahocevar · 2013-06-21T15:48:26Z

@twpayne, despite your concerns, what would get worse if we merge this now? It is an improvement to what we have, it is self contained (i.e. does not touch other files), and can be revisited any time later. I could comment on all your points as well and find counter-arguments for most everything you say, but I'll rather enjoy the warm summer evening.

twpayne · 2013-06-21T15:57:42Z

I would be interested to hear the counter-arguments, but no rush.

ahocevar · 2013-06-21T16:02:49Z

I would be interested in seeing your 'Please merge, but let's continue to discuss this'. No rush either, but I'd be more motivated to explain things like how I envision editing without redrawing tiles all the time and labeling when rotating after merging. And that discussion should rather go on the mailing list or a dedicated ticket.

twpayne · 2013-06-21T16:04:48Z

Let's continue to discuss this - either here or on the mailing list - but I don't think that this code should be merged. In my opinion, this is a very messy solution that does not correctly solve the issue.

twpayne · 2013-06-21T16:08:54Z

This blog post may be helpful:
https://www.scirra.com/blog/112/remember-not-to-waste-your-memory

#764 is also relevant.

ahocevar · 2013-06-21T16:17:24Z

@twpayne, this is getting ridiculous. From #764:

The problem is related not only to the vector example. Any example with more than one layer or any overlays will most likely crash in about ~10 sec. extensive of use - zooming in/out, scrolling, rotating.

We're all psyched to see fully styled vector layers in WebGL, updated instantly with every frame, and with no images stored anywhere. But until we get there, we need a working alternative. The code to improve Canvas rendering is here, it's only waiting to be merged.

tschaub · 2013-06-21T16:29:08Z

rendering vector data into raster tiles is fundamentally flawed

It would be good for you to pick up this argument elsewhere @twpayne; it is counterproductive here. Rendering vector data to raster tiles is exactly the approach that has resulted in an explosion of popularity in slippy maps over the past ~10 years.

I'm not saying you don't have valid points to make about alternatives in client side rendering (the approach has proven itself hugely successful on the server, but we're only starting to experiment with it here on the client). But I think injecting your arguments on this issue is only causing frustration.

Let's try to work together on an alternative that can be compared for performance. Iterating through thousands of features with every mousemove has performance implications as well.

tschaub · 2013-06-21T16:37:37Z

@ahocevar one approach that I've been curious to try is to cache a single resolution only. This would support animated zooming (use previous resolution until animation ends) and minimizes redrawing on panning. I haven't fully reviewed your work here, but I'm curious what you think about the alternative.

ahocevar · 2013-06-21T16:41:05Z

Sounds like a great approach @tschaub. We know now that the rendering itself is fast, we only don't want to do it during animations and interactions. So after your suggested change, whenever the view settles, we get a sharp rendered image at a native resolution, and don't need the overhead of a tile grid with multiple resolutions and a messy way to update the tile cache. Brilliant. Updating the pull request now.

twpayne · 2013-06-21T23:00:45Z

@ahocevar one approach that I've been curious to try is to cache a single resolution only.

This makes more sense. Maybe vector layers should work more like single image layers than tile layers. It would be nice to be able to re-use the existing image layer renderers. Perhaps a vector layer could pretend to be a single image source? This can easily be hidden from the user (i.e. when a renderer needs to create a layer renderer for vector source, it internally creates a vector to single image layer bridge and passes this single image source to a single image layer renderer).

twpayne · 2013-06-21T23:07:56Z

Note that rendering vectors to a single image (or caching only a single resolution) will still suffer from the problems described in the aforementioned blog post:

Mobile overdraw
While we're on the subject of mobile limitations, it's worth mentioning some mobile devices can only draw each pixel on the screen 3 times per frame and still reach 60 FPS. In other words, if you have four window-sized images on screen, you cannot reach 60 FPS on such a device. (Note Construct 2 renders scenes from the back to the front, so four overlapping objects are all still rendered.) Perhaps surprisingly, this includes transparent pixels. The GPU processes the full rectangle of a texture regardless of the content, and a transparent pixel still uses up your drawing budget, so four window-sized transparent images would still not reach 60 FPS. Similarly with memory usage, this is a device hardware limitation and you'll have the same limitation with any framework.

Basically, we should avoid vector layer strategies that include caching pixels that are transparent. A good thought exercise is to consider the cost of rendering and compositing the vector layer if it only contains a few - or even no - features.

tschaub · 2013-06-21T23:26:30Z

It would be a straightforward enhancement to stop caching tiles that don't intersect any features. This should be handled in a separate issue (see #808).

tschaub · 2013-06-23T16:19:56Z

src/ol/renderer/canvas/canvasvectorlayerrenderer.js

@@ -1,5 +1,6 @@
 goog.provide('ol.renderer.canvas.VectorLayer');

+goog.require('goog.array');


Unused require.

tschaub · 2013-06-23T16:27:36Z

This is awesome @ahocevar. Please address the minor comments above (about goog.require) and then merge.

Instead, we create a new tile grid whenever renderFrame is called, no animation is active, and the resolution is not in the tile grid already. This gives better rendering results because we get vector tiles at native resolutions.

@elemoine

Do not use a fixed tile grid for vector rendering. r=@elemoine,@tschaub,@twpayne

mlegenhausen · 2013-06-24T07:37:14Z

It seems that after this change the vector rending on mobile devices is broken. Test in Chrome Beta on Android.

ahocevar · 2013-06-24T09:39:02Z

@mlegenhausen On my Android device, this change is not to blame. I do get points rendered, but no lines and polygons. And I went back through several revisons - all were the same. Unfortunately I cannot test on the native browser, because there are many more things broken. Repurposing today to mobile testing and fixing.

ahocevar · 2013-06-24T11:07:38Z

@mlegenhausen What is the issue you are seeing? One that I am seeing is that when pinch-zooming, the vector layer does not get rendered at the new resolution, and is scaled instead. This is caused by #812.

mlegenhausen · 2013-06-24T12:51:25Z

No blame at all. Great work!
Yeah that was exactly what I saw. Thanks for the hint.

jacobstanley · 2013-06-24T16:07:23Z

I'm having a few minor issues which I'm making tickets for, but this change has really improved usability for vectors. Great work!

elemoine reviewed Jun 20, 2013
View reviewed changes

twpayne reviewed Jun 21, 2013
View reviewed changes

ahocevar mentioned this pull request Jun 21, 2013

Vector tiles disappear during zoom animation #350

Closed

tschaub reviewed Jun 23, 2013
View reviewed changes

ahocevar added 5 commits June 23, 2013 18:30

Do not use a fixed tile grid for vector rendering

3afd8b4

Instead, we create a new tile grid whenever renderFrame is called, no animation is active, and the resolution is not in the tile grid already. This gives better rendering results because we get vector tiles at native resolutions.

Separate method for updating the tile cache

0aacf19

Do not add resolutions while interacting

aa62764

Use binaryInsert instead of sort

89c23b5

Tile grid and cache for one resolution only

f20e600

ahocevar added 5 commits June 23, 2013 18:31

Do not render tiles during animation and interaction

61fcbd5

Adding comment about MIN_RESOLUTION

1c5f466

Fixing requires

27c23f6

Simpler tile coord access now that we only have one resolution

af0764b

Need to take units into account

5cc3e18

ahocevar added a commit that referenced this pull request Jun 23, 2013

Merge pull request #799 from ahocevar/dynamic-canvas-tilegrid

b30dcdd

Do not use a fixed tile grid for vector rendering. r=@elemoine,@tschaub,@twpayne

ahocevar merged commit b30dcdd into openlayers:master Jun 23, 2013

ahocevar deleted the dynamic-canvas-tilegrid branch June 23, 2013 18:15

jacobstanley mentioned this pull request Jun 24, 2013

Vector rendering broken for projections which do not specify a name for units #814

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not use a fixed tile grid for vector rendering #799

Do not use a fixed tile grid for vector rendering #799

ahocevar commented Jun 19, 2013

ahocevar commented Jun 20, 2013

elemoine Jun 20, 2013

ahocevar Jun 20, 2013

twpayne commented Jun 21, 2013

elemoine commented Jun 21, 2013

twpayne Jun 21, 2013

ahocevar Jun 21, 2013

tschaub Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne Jun 21, 2013

ahocevar Jun 21, 2013

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

tschaub commented Jun 21, 2013

tschaub commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

twpayne commented Jun 21, 2013

tschaub commented Jun 21, 2013

tschaub Jun 23, 2013

tschaub commented Jun 23, 2013

mlegenhausen commented Jun 24, 2013

ahocevar commented Jun 24, 2013

ahocevar commented Jun 24, 2013

mlegenhausen commented Jun 24, 2013

jacobstanley commented Jun 24, 2013

		@@ -1,5 +1,6 @@
		goog.provide('ol.renderer.canvas.VectorLayer');

		goog.require('goog.array');

Do not use a fixed tile grid for vector rendering #799

Do not use a fixed tile grid for vector rendering #799

Conversation

ahocevar commented Jun 19, 2013

ahocevar commented Jun 20, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twpayne commented Jun 21, 2013

elemoine commented Jun 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahocevar commented Jun 21, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

twpayne commented Jun 21, 2013

ahocevar commented Jun 21, 2013

tschaub commented Jun 21, 2013

tschaub commented Jun 21, 2013

ahocevar commented Jun 21, 2013

twpayne commented Jun 21, 2013

twpayne commented Jun 21, 2013

tschaub commented Jun 21, 2013

Choose a reason for hiding this comment

tschaub commented Jun 23, 2013

mlegenhausen commented Jun 24, 2013

ahocevar commented Jun 24, 2013

ahocevar commented Jun 24, 2013

mlegenhausen commented Jun 24, 2013

jacobstanley commented Jun 24, 2013