Add a renderBuffer option to ol.layer.Vector

[elemoine]

This PR fixes a the bug reported in #2851 and a renderBuffer option to ol.layer.Vector.

The bug reported in #2851 can be easily reproduced using the icon example. Just move the map north until the icon hits the bottom of the viewport: the icon disappears as soon as the icon position ([0, 0]) is outside the viewport.

This PR fixes the bug by removing the ol.extent.intersects tests in the ReplayGroup's replay functions. This means Replay objects are now unconditionally replayed, even when their envelope does not intersect the viewport extent.

Also, currently, we do not recreate a new ReplayGroup when the extent of the current ReplayGroup contains the viewport extent. This means that we may have rendering issues (symbols/icons not drawn) when the viewport approaches the boundary of tthe ReplayGroup extent.

This PR addresses this problem by using not re-creating the batch only in the case where the ReplayGroup extent equals the viewport extent. But note that we still do not create new ReplayGroup's during animations and interactions.

And, at last, this PR adds a renderBuffer option to ol.layer.Vector that allows specifying the size (in pixels) of the rendering buffer around the viewport extent. The default value is 100 pixels, but larger values can be used when using large icon sizes or line widths. This option is somewhat similar to MapServer's tile_map_edge_buffer.

Please review.

Fixes #2851.

[fredj]

LGTM, thanks !

[tschaub]

This looks great to me. Very nice description of the problem and the solution. I still think we could check for contains since we are always comparing buffered extents - I agree we would have a problem if we only checked to see whether the rendered extent contained the unbuffered frame extent. But as I mention above, given the other conditions, there are few cases where the batch extent contains but does not equal the buffered frame extent.

Interested if I'm missing something, but please merge in any case.

[elemoine]

I changed the commit history. Here the exchange @tschaub and I had on some previous commit:

@tschaub:

Isn't this.renderedExtent_ always buffered? I see how we would have rendering issues if we only recreated the batch when the unbuffered rendered extent didn't contain the buffered frame extent. But I don't immediately see how we'd have problems if we only recreate the batch if the buffered rendered extent doesn't contain the buffered frame extent.

That said, because we're also checking for matching resolution here, I think we could only have a buffered rendered extent that contains but does not equal the buffered frame extent when the viewport size changes. So, while I think we could still be using ol.extent.containsExtent here, it's a minor optimization.

Curious if you see a case where contains would fail (assuming this.renderedExtent_ is never set to an unbuffered frame extent).

@elemoine:

You are totally correct. Previously (in the current master branch) we only recreate the batch if the (buffered) rendered extent does not contain the (unbuffered) viewport extent. But, with my changes, we compare the rendered extent to the buffured viewport extent. So you're absolutely right, I could still use containsExtent rather than equal. And as you said this is a small optimization for the case where the viewport size changed. This may also be a small optimization for the case where the renderBuffer changes (which is not possible at the moment because there is not setter). Anyway good comment, I'll make the change before merging.

https://github.com/elemoine/ol3/commit/567316c8d9fc3fd9c1d2dff933a7b92c95e10106

[tschaub]

Unfortunately, it looks like ddc51ee messes with the select interaction when it is configured to respond to mousemove (see #3081).