Switch to basic canvas operations to fix circle stroke position

[orangemug]

This PR switches over to basic canvas rendering for circles to fix stroke position. Previously the stroke was positioned mid way along the line like so

Before

After

This also means that circles are visually the correct size. You can see the issue in these before screenshots from my (modified) spec helper, you can see that the total radius of the circle is incorrect because of the circle stroke sitting mid-way on the circle fill (rather than outside)

Because we're now using basic canvas operations I also added support for circle-blur

@ahocevar if you're happy with the approach, let me know and I'll give it another good test prior to making it ready for merge.

[ahocevar]

Although I think this will cause a decrease in performance when many circles are rendered, I'm in favor of this pull request because I don't think circles see much use compared to sprite symbols.

[orangemug]

Cool, thanks @ahocevar I'll get on it today.

I think the performance would probably be very similar. Those canvas operations are probably as simple a we can make them so openlayer core would have to be doing something very similar (not 100% sure on that) and we still do caching, like before. I think the only difference would be the calling of a function + a bit of additional math.

ol-mapbox-style/src/stylefunction.js

Lines 1182 to 1192 in e392ac8

	style.setRenderer(([x, y], renderOpts) => {
	const r = circleRadius * renderOpts.pixelRatio;
	const sw = circleStrokeWidth * renderOpts.pixelRatio;
	const cb = circleBlur * renderOpts.pixelRatio;
	const xo = x + circleTranslate[0] * renderOpts.pixelRatio;
	const yo = y + circleTranslate[1] * renderOpts.pixelRatio;
	const w = r * 2 + sw * 2 + sw / 2 + cb * 2;
	const h = r * 2 + sw * 2 + sw / 2 + cb * 2;
	let bitmap = iconImageCache[cache_key];

I assume the JS engine optimisation would do it's thing after a couple of thousand runs though making it a tiny perf loss.

[ahocevar]

Another thing to consider is whether OpenLayers hit detection works with circles like these.

[orangemug]

Good shout on hit detection @ahocevar, I've added that now and all seems to be good. I've been testing on a few different devices using a modified ./examples/geojson-inline.js with the following added

apply('map', 'data/geojson-inline.json').then((map) => {
  map.on('pointermove', (evt) => {
    const features = [];
    map.forEachFeatureAtPixel(
      evt.pixel,
      (feature) => {
        features.push(feature);
      },
      {hitTolerance: 0}
    );
    console.log('features=', features);
  });
});

This should be good to review/merge.

[ahocevar]

I think this could be simplified by creating a separate canvas for the circle, and use it as image in an icon style. Then the whole hit detection code could go away.

[orangemug]

I think this could be simplified by creating a separate canvas for the circle, and use it as image in an icon style. Then the whole hit detection code could go away.

I'm not sure I understand @ahocevar. The hit detection needs to use a different render function than the normal render code. This is because a blur will have opacity from 1 => 0 at the edges. However the hit detection need to be somewhere between those two values.

So for example we don't want hovering over an opacity of 0.01 at the edge of a blur to trigger a "hit". In my tests cb * 3 (where cb is circle-blur) seems to give the best results closest to maplibre (I manually tested this).

Thoughts?

[ahocevar]

@orangemug I'm talking about a simpler approach altogether - instead of using canvas operations with a custom renderer, you could generate the circle once in a separate canvas, and use the separate canvas as icon in an Icon style - same thing as for the icons that come from a sprite sheet, only that the icon is a canvas and does not come from the sprite sheet.

[orangemug]

@orangemug I'm talking about a simpler approach altogether

Yeah @ahocevar I think I understand that. Sorry if I'm missing something, but how would your approach deal with blur (as mentioned in #1059 (comment) as why I took this approach)

Blur needs to use a separate approach for hit boxes, which is the work in this PR. Also with regards to "circle once in a separate canvas" we already cache reused circles with the iconImageCache so we're not duping the work for the same size/color/outline/etc... for circles.

Does that make sense, or am I missing something all together 😄

[ahocevar]

The thing you're missing is that also for blur, you'd be creating a separate canvas (or image bitmap) which serves as an icon image in an Icon style. Hit detection will then just work on the image data, like with other icons.

I had misread the code and thought your custom renderer would draw the circle to the canvas each time. Now that I see you're creating an ImageBitmap already, it is even simpler to change your code to what I suggest.

Instead of calling context.drawImage() with the bitmap you create, you can use the bitmap as icon for an Icon style.