Fix for reprojecting opaque tile sources

[petrsloup]

The tile sources that are marked as 'opaque' (to benefit from rendering optimizations) are sometimes rendered incorrectly when reprojected -- the resulting reprojected tiles are not always opaque (e.g. projection boundaries, deformations, ...) and tiles from other zoom levels "bleed-through".

This PR changes getOpaque to return false when reprojecting.

[tschaub]

The source.getOpaque(projection) method is now a bit hard to explain. And the opaque value is deceptive. Why not move the projection equivalence check here? Maybe something like:

var opaqueAndNotReprojecting = ol.ENABLE_RASTER_REPROJECTION ?
    ol.proj.equivalent(tileSource.getProjection(), projection) : tileSource.getOpaque();

And then use opaqueAndNotReprojecting below. Cumbersome name, but at least it describes what it is.

[petrsloup]

So far, most of the tile reprojection handling is in ol.source.TileImage so I added it there to keep it in all one place.
The renderer has no special handling related to reprojection and the opaqueAndNotReprojecting would make sense only for ol.source.TileImage (not for other subclasses of ol.source.Tile).

Alternatively, we could rename getOpaque to getOpaqueForProjection (or keep getOpaque unchanged and add getOpaqueForProjection) to make it more clear (and consistent with getTileGridForProjection and getTileCacheForProjection methods).

I have no strong opinion on this, so I'm not opposed to moving the projection equivalence check to the canvastilelayerrenderer.js if others also think it would be better. Opinions?

[tschaub]

The main concern that I have is that I can't make any sense of source.getOpaque(projection) (or getOpaqueForProjection). How would you explain what that method means? Opacity and projection are unrelated. So it suggests to me that "opacity" is being overloaded in a bad way (how many additional meanings should be packed into the same word?)

[tschaub]

Re-reading your original description, it sounds like you are talking about the reprojection resulting in non-opaque pixels being rendered.

Can you provide a screenshot to help illustrate the issue?

[petrsloup]

@tschaub Sorry the problem description was not clear. I'll try to explain it better this time.

Some sources are marked as "opaque", which the canvastilelayerrenderer uses as a hint to perform certain optimizations (it skips clearRect calls since the content is to be fully overwritten anyway).

When reprojecting for example MapQuest source to some projections, the new tiles (created by the reprojection) can be partially transparent although the original tiles are always opaque:

(example from current master)
The error on the bottom is caused by limited extent of the target projection, while the error on the top is caused by limited extent of the source projection (and tile grid).
In both cases, the canvastilelayerrenderer expects the tiles to be fully opaque (as hinted by getOpaque) and does not clear the old content in the canvas (when zooming for example) -- the opaque hint makes sense only for the original "data" projection and is not reliable when reprojecting.

[tschaub]

Thanks for the clarified description and illustration of the problem @petrsloup.

So ideally the canvas renderer could know source.getOpaque(extent, projection). But I know that would be a more involved change for potentially little gain.

I agree it is nice to contain the concerns about reprojecting. So I think this makes sense to merge as it is.

Given your screenshot above, this looks like a great place for a rendering test. I'm not sure how sensitive the assertions would be to changes in the reprojection implementation, so I don't have a sense for how hard these would be to maintain (or create in the first place).

[petrsloup]

@tschaub I tried to create the rendering test for this, but it proved to be quite complicated. The issue is only visible under certain conditions (after interaction -- at least zooming), so the test needs to have several "steps" and a lot of trial and error is required to get the test constants right.

I'm afraid I don't have enough time to look at the test now, so (unless there are other comments or notes) I think we should merge this as it is to have the fix in v3.13.0.

[tschaub]

@petrsloup I agree this should be merged. Thanks for the fix and for the additional explanation.

[petrsloup]

Thanks @tschaub