Batch polygon and circle fills and strokes #5196

ahocevar · 2016-04-07T16:13:28Z

Better solution than the reverted one from #5149.

ahocevar · 2016-04-07T16:36:30Z

Opened for discussion:

The problem with batching fills and strokes is that overlapping polygons and circles with transparency won't have increased opacity. See #5179 (comment). Maybe there is a way to achieve fill overlap when applying a fill to a path that has overlaps, but I haven't found one.

One way to deal with this would be to make rendering behaviour configurable, so users can decide whether they want increased performance and no increased opacity on overlapping features with transparency, or features with true visual overlap at the cost of poorer performance.

Ideas?

ahocevar · 2016-04-07T18:01:56Z

How about this: when creating the replay, we check if there is any opacity. If so, we do not batch. Otherwise we do. I can update the pull request easily for a proof of concept.

ahocevar · 2016-04-07T21:22:48Z

I think this is a nice approach. Ready for review.

bjornharrtell · 2016-04-12T21:41:38Z

While I agree this approach is a nice default that works, I would find an option to force the batching always on useful in cases where I don't care about the opacity-correctness.

tschaub · 2016-04-14T11:04:35Z

src/ol/render/canvas/canvasreplay.js

@@ -1440,13 +1484,20 @@ ol.render.canvas.PolygonReplay.prototype.setFillStrokeStyle = function(fillStyle
    var fillStyleColor = fillStyle.getColor();
    state.fillStyle = ol.colorlike.asColorLike(fillStyleColor ?
        fillStyleColor : ol.render.canvas.defaultFillStyle);
+    if (!this.transparency && ol.color.isRgba(state.fillStyle)) {


Since ol.color.isRgba() is only used to test if alpha is not 1, it would be simpler just to have ol.color.isOpaque() or something (rather than first testing if it is RGBA and then creating an array and then testing the alpha value).

tschaub · 2016-04-14T11:30:41Z

I don't want to rain on the parade, but I'm dubious about the effort to batch fills and strokes in this way. Here is a picture with batched fills and strokes:

I don't think this is what people want.

I'm also curious about how this affects what is reported in #4232, since I think that with the changes here, things are not being batched (because this.transparency is true whenever the red value is not 1, see #5196 (comment)).

tschaub · 2016-04-14T11:37:06Z

I can imagine we could gain performance when there is no fill or no stroke. In those cases, we could defer stroking or filling (respectively) until the end. We could also delay stroking or filling until a fill or stroke is encountered (no need to stroke until you are going to fill and no need to fill until you are going to stroke). But these performance gains would be limited to pretty narrow cases.

ahocevar · 2016-04-18T08:43:09Z

Thanks @tschaub and @bjornharrtell for the valuable feedback. Based on that, I got rid of the opacity guessing game and introduced a new overlaps option for ol.source.Vector and ol.source.VectorTile, which allows users to decide whether the fill and stroke optimisations should be applied. Does this look better to you now?

bjornharrtell · 2016-04-18T13:00:38Z

Looks good to me. I like the note on which kind of data this works on, another argument for my mission to convince people to work with non-overlapping topological data.

tschaub · 2016-04-19T16:11:49Z

src/ol/render/canvas/canvasreplay.js

+  var pendingFill = 0;
+  var pendingStroke = 0;
+  var batchSize =
+      this.instructions != instructions || this.overlaps ? 0 : 200;


Curious about the 200 here. Did you find that performance is degraded if the batch size gets too big? It might be nice to document the justification for this default on its own line.

tschaub · 2016-04-19T16:14:40Z

This sounds like a nice solution @ahocevar. It looks like your tests may cover it, but it would be even clearer to see rendering tests for overlaps: false with and without changing stroke and fill styles (to make sure we always properly stroke/fill when the styles change).

ahocevar · 2016-04-22T11:56:32Z

Thanks for the review @tschaub. I have addressed your comments. Can you take another look please?

ahocevar · 2016-04-27T07:52:30Z

Is this good to merge now @tschaub?

Instead of deciding whether to batch fills and strokes by looking at the opacity of the style, we now rely on user input.

ahocevar · 2016-08-23T10:08:08Z

Rebased one more time.

bartvde · 2016-08-23T14:31:16Z

externs/olx.js

@@ -4416,6 +4417,17 @@ olx.source.VectorTileOptions.prototype.logo;


 /**
+ * This source may have overlapping geometries. Default is `true`. Setting this
+ * to `false` (e.g. for sources with polygons that represent adminstrative


administrative

bartvde · 2016-08-23T14:50:49Z

LGTM @ahocevar

ahocevar · 2016-08-24T10:02:48Z

Thanks for the reviews @tschaub and @bartvde.

probins · 2016-08-24T10:06:08Z

sorry for being late, but shouldn't this addition to the API be documented in the release notes?

ahocevar · 2016-08-24T10:07:54Z

sorry for being late, but shouldn't this addition to the API be documented in the release notes?

Changes like this can be added to the release notes by the release manager when the release is created.

jeroenvheel · 2016-08-29T17:51:32Z

the performance boost with this change is amazing !!

mstrop · 2016-11-28T11:54:33Z

This says the issue #4232 is closed by this issue, I but still have terrible performance issue using IE (11 or Edge) - FF is a bit better, but not much. I use OL 3.19 version. Is there any progress on this?

ahocevar · 2016-11-28T12:01:57Z

@mstrop Have you configured your source with overlaps: false?

mstrop · 2016-11-28T13:17:44Z

Yep, both - with and without. The difference is minimal. In addition to this, having the source defined as overlaps: false is causing display bugs for certain zooms.

ahocevar · 2016-11-28T14:06:38Z

@mstrop Looks like your source does not have topological data, so you cannot use the overlaps: false option anyway. I think you'll have to apply different optimisations here, because your use case is not one that this fix was made for.

mstrop · 2016-11-28T15:19:46Z

@ahocevar ups, it's not a good message. Is there a chance this issue will be solved in future?

ahocevar · 2016-11-28T15:24:01Z

@mstrop The only two places where I see additional potential for library side performance optimization is text and regular shape (including circle) rendering.

In your case, however, I suspect that you could optimize the style functions for your vector layers on the application level.

mstrop · 2016-11-28T15:36:06Z

@ahocevar thanks a lot.

mstrop · 2016-12-13T20:33:59Z

@ahocevar sry, one more question - I was thinking about your comment "I suspect that you could optimize the style functions for your vector layers on the application level". What is relation between a styling function and forEachFeatureAtPixel method? I use forEachFeatureAtPixel inside pointer move event as I need to change cursor in case it hovers above a feature. So the styling function is not called as the map is not being redrawn. Or am I missing anything?

ahocevar · 2016-12-13T20:51:37Z

Even if you are only using forEachFeatureAtPixel for changing the cursor, without highlighting any features on the map, the style function will be called. Because for hit detection, features near the cursor will be rendered to a hidden small canvas to detect which features are rendered at the cursor location.

mstrop · 2016-12-13T20:58:21Z

@ahocevar thanks a lot for your very prompt response. I am really sorry, but I don't agree with you - I just made a quick test - I placed console.log(new Date().getTime()); as the first row of my styling function - sure, it overloads my console when I move the map, but there is no single record when the map is static and I just move the cursor above it.

ahocevar · 2016-12-13T22:25:29Z

My mistake @mstrop. The style function is called when creating the replay, not when replaying. And during mousemoves, it is only replayed.

However, without seeing your code, it is hard to guess what could be the culprit. Does your vector source use a spatial index? Have you tried with a smaller renderBuffer configured on your vector layer? For polygons, you can set it to half of the stroke width instead of the default of 100, and that will give you a massive performance gain, especially when your polygons are small at the viewed resolution.

ahocevar force-pushed the batch-fill-stroke branch 2 times, most recently from e33d467 to a6fd547 Compare April 7, 2016 16:35

ahocevar force-pushed the batch-fill-stroke branch 2 times, most recently from 5391a0d to 81498bf Compare April 7, 2016 21:11

ahocevar changed the title ~~[WIP] Batch polygon and circle fills and strokes~~ Batch polygon and circle fills and strokes Apr 7, 2016

ahocevar force-pushed the batch-fill-stroke branch 5 times, most recently from 3407ceb to 59d6066 Compare April 8, 2016 10:18

tschaub reviewed Apr 14, 2016
View reviewed changes

ahocevar force-pushed the batch-fill-stroke branch 2 times, most recently from 695484d to 259b601 Compare April 18, 2016 09:01

tschaub reviewed Apr 19, 2016
View reviewed changes

ahocevar force-pushed the batch-fill-stroke branch from 259b601 to 5dc2043 Compare April 22, 2016 11:53

ahocevar force-pushed the batch-fill-stroke branch from 5dc2043 to 232f9e3 Compare April 22, 2016 11:57

ahocevar force-pushed the batch-fill-stroke branch 2 times, most recently from 0b56136 to b521675 Compare April 28, 2016 14:02

ahocevar added 2 commits August 23, 2016 12:05

Batch polygon and circle fills and strokes

eed5255

Introduce new overlaps option for Vector and VectorTile sources

7e940e6

Instead of deciding whether to batch fills and strokes by looking at the opacity of the style, we now rely on user input.

ahocevar force-pushed the batch-fill-stroke branch from f50dfc1 to 1b6eea6 Compare August 23, 2016 10:07

bartvde reviewed Aug 23, 2016
View reviewed changes

ahocevar force-pushed the batch-fill-stroke branch from 1b6eea6 to 6daf7b2 Compare August 24, 2016 09:41

Additional tests and code comments

395793b

ahocevar force-pushed the batch-fill-stroke branch from 6daf7b2 to 395793b Compare August 24, 2016 09:44

ahocevar merged commit b7c84b2 into openlayers:master Aug 24, 2016

ahocevar deleted the batch-fill-stroke branch August 24, 2016 10:02

ahocevar mentioned this pull request Aug 29, 2016

Gap between stacked polygons #5303

Closed

jordipiqueres mentioned this pull request Sep 6, 2018

Very slow performance on hover features in ff & ie #8592

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch polygon and circle fills and strokes #5196

Batch polygon and circle fills and strokes #5196

ahocevar commented Apr 7, 2016 •

edited

Loading

ahocevar commented Apr 7, 2016

ahocevar commented Apr 7, 2016

ahocevar commented Apr 7, 2016

bjornharrtell commented Apr 12, 2016

tschaub Apr 14, 2016

tschaub commented Apr 14, 2016

tschaub commented Apr 14, 2016

ahocevar commented Apr 18, 2016 •

edited

Loading

bjornharrtell commented Apr 18, 2016

tschaub Apr 19, 2016

tschaub commented Apr 19, 2016

ahocevar commented Apr 22, 2016

ahocevar commented Apr 27, 2016

ahocevar commented Aug 23, 2016

bartvde Aug 23, 2016

bartvde commented Aug 23, 2016

ahocevar commented Aug 24, 2016

probins commented Aug 24, 2016

ahocevar commented Aug 24, 2016

jeroenvheel commented Aug 29, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

mstrop commented Dec 13, 2016

ahocevar commented Dec 13, 2016

mstrop commented Dec 13, 2016

ahocevar commented Dec 13, 2016 •

edited

Loading

Batch polygon and circle fills and strokes #5196

Batch polygon and circle fills and strokes #5196

Conversation

ahocevar commented Apr 7, 2016 • edited Loading

ahocevar commented Apr 7, 2016

ahocevar commented Apr 7, 2016

ahocevar commented Apr 7, 2016

bjornharrtell commented Apr 12, 2016

tschaub Apr 14, 2016

Choose a reason for hiding this comment

tschaub commented Apr 14, 2016

tschaub commented Apr 14, 2016

ahocevar commented Apr 18, 2016 • edited Loading

bjornharrtell commented Apr 18, 2016

tschaub Apr 19, 2016

Choose a reason for hiding this comment

tschaub commented Apr 19, 2016

ahocevar commented Apr 22, 2016

ahocevar commented Apr 27, 2016

ahocevar commented Aug 23, 2016

bartvde Aug 23, 2016

Choose a reason for hiding this comment

bartvde commented Aug 23, 2016

ahocevar commented Aug 24, 2016

probins commented Aug 24, 2016

ahocevar commented Aug 24, 2016

jeroenvheel commented Aug 29, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

ahocevar commented Nov 28, 2016

mstrop commented Nov 28, 2016

mstrop commented Dec 13, 2016

ahocevar commented Dec 13, 2016

mstrop commented Dec 13, 2016

ahocevar commented Dec 13, 2016 • edited Loading

ahocevar commented Apr 7, 2016 •

edited

Loading

ahocevar commented Apr 18, 2016 •

edited

Loading

ahocevar commented Dec 13, 2016 •

edited

Loading