Clear the canvas on layer extent changes

[tschaub]

Things I like about this solution:

• The layer renderer listens for changes on the layer. This is far more direct than having the map listen for layer group changes and then call map.render(), as by the time we get to preparing the frame in the layer renderer, we know nothing about what changed. And we have to store extra state on the layer renderer in order to figure out what work we need to do.

Things I don't like about this solution:

• Clearing the canvas and unsetting rendereredCanvasZ_ is a bit awkward. It would be nice to have a forgetEverythingAndStartOver method. But I'm not sure where else this would get used yet.
• The WebGL renderer doesn't really work well with layers with limited extents. This doesn't help there.
• Both the Canvas and the DOM renderer will render tiles from a higher zoom level while zooming out. So, when zooming from 2 to 1, tiles from 2 will be rendered when we get to 1 if tiles from 1 are not loaded. The result is that when you change the layer extent and zoom out, you can occasionally see tiles from the previously rendered zoom level that are outside the new extent. I feel like there is a bug lurking here.

Input welcome.

Fixes #2731.

[tschaub]

I feel like there is a bug lurking here.

Indeed there is a bug - see #2758. Currently, we look for and conditionally render 9 child tiles instead of 4 to cover a loading parent tile when zooming out.

[elemoine]

Currently, renderers receive orders through the frame state. They are "frame state driven". This change is a departure from that design. With this change the canvas tile renderer is both "frame state driven" and "layer event driven". Just wanted to point this out.

Also, on the code itself now: when the layer is removed from the map goog.dispose is called on the corresponding layer renderer. But the change:extent listener is not unregistered, so the layer will still have a reference to the layer renderer. Think about the case where a layer is added then removed, added then removed, etc. The number of change:extent listeners will keep increasing. It's a leak. So the layer renderer should implement disposeInternal and unregisters its listeners. The "frame state" approach has the advantage of avoiding this kind of "event listeners" housekeeping.

An alternative would be to store the "current layer extent" in the renderer instance and clear the canvas when ol.extent.equals(this.currentLayerExtent_, layerState.extent) is false. But I know this is an extra state in the layer renderer…

I'd say, use the approach you like best, but if the change:extent approach is your favorite then the listener should be unregistered when goog.dispose is called on the layer renderer. In case you're interested, for consistency reasons, I'd stick to the "frame state" approach and use an extra state in the layer renderer. I'm not opposed to the "event driven" approach, but if we go that path, I think we should change the entire architecture (with a major refactoring) instead of making a special case for layer extent changes.

[tschaub]

But I know this is an extra state in the layer renderer…

Right. This is what I don't like about the "fire generic events and let all listeners reconstruct what happened" approach.

[tschaub]

With this change the canvas tile renderer is both "frame state driven" and "layer event driven"

With or without this change, rendering is "driven" by modifications to layer state. Layer events trigger rendering. The frame state doesn't "drive" anything - it represents a ton of state. Listening for layer events in the map and then calling render makes it so renderers (potentially - see below) have to store a ton of state themselves so they can decide what to do with the frame state object. I'd like to start reworking things so listeners are registered closer to where they are needed to reduce the amount of state that has to be stored for comparison. But I'm not going to do that here.

use the approach you like best

I have. I see this as an incremental change in the right direction.

the listener should be unregistered when goog.dispose is called on the layer renderer

Good catch. Fixed with e1ee347.

By "potentially" above I mean that if we really wanted things to be efficient with the current design, layer renderers would need to store not only the previous layer state that comes with the frame state, but all layer "properties" so they could ignore changes that don't affect rendering.

[elemoine]

The array of listener keys could be created by ol.renderer.Layer (the base constructor), and disposeInternal could be implemented on that constructor's prototype. In this way we provide a more generic way for listening to layer events. Just a comment, not requesting to do this now.

[elemoine]

Please merge.