feat: add multi-world support to Polygons and Polylines & refactoring

[monsieurtanuki]

What

• Now Polygons and Polylines support multi-world.
• A good opportunity to refactor with Circles, and to imagine other layers like "painted Markers".
• Fixed a side-effect bug regarding polygon labels, that had a strange behavior for polygons around -180/180 (like, centered on longitude 0).
• Refactored too the computation of "meter to pixel" computation, previously computed differently in Circle and Polyline

Screenshots

Files

New file:

• multi_world_layer_helper.dart: Helper for multi world: e.g. draw and hitTest on all world copies.

Impacted files:

• internal_hit_detectable.dart: new MultiWorldLayerHelper field
• label.dart: implemented position computation for new value PolygonLabelPlacement.centroidWithMultiWorld
• multi_worlds.dart: added a PolygonLayer and a PolylineLayer
• offsets.dart: added a double shift = 0 parameter for getOffset and getOffsetXY
• circle_layer/painter.dart: refactored using new class MultiWorldLayerHelper
• polygon_layer/painter.dart: implemented the multi world using new class MultiWorldLayerHelper
• polyline_layer/painter.dart: implemented the multi world using new class MultiWorldLayerHelper
• polygon.dart: new value for PolygonLabelPlacement - centroidWithMultiWorld, as a fix for side-effects around the -180 or 180 longitude

[JaffaKetchup]

LGTM, thanks once again!

Before I merge, it would be good to get your (@monsieurtanuki) opinion on the changes I've made.

• Converted your new helper object into a mixin, and renamed it, as using it doesn't necessarily force the layer to support multi worlds
• Removed the HitTestRequiresCameraOrigin mixin, as that functionality has been absorbed into your new object/mixin
• Converted the existing HitDetectablePainter into a mixin
• Removed the two different, but very similar, implementations of the method in your object that allowed iteration across the worlds, and merged them into one method - essentially before, where the second one had a callback returning true, the callback should return false, and false -> null

I changed your object into a mixin as I thought it fit better with the inheritance system we had in place, and it would allow for the removal of "helper." prefixes, and allowed for better removal of the HitTestRequiresCameraOrigin mixin. It also meant we could enforce the setting of the canvas size/viewport rect.

Of course, it could be taken a step further if we merge some things together. elementHitTest now always repeats across the worlds, so we could take that out of each layer - for example. We'd then get one bigger mixin. Wdyt?

[monsieurtanuki]

Hi @JaffaKetchup! It looks like you understand my own code better than I do ;)
As I'm less flutter-ish than you are, I'll let you use mixin, which I'm not used to.

Some comments

I was not convinced by your extends CustomPainter, until I saw

@mustCallSuper
@mustBeOverridden
void paint(...

which is indeed much safer.

Probably due to the mixin, you repeat the declaration of the fields in the 3 layers, and I don't find this very elegant. Would a mixin class help? I don't know.

  @override
  final MapCamera camera;

  @override
  final LayerHitNotifier<R>? hitNotifier;

The problematic part is your merging of both "do-it-in-all-worlds" methods. I understand the idea of refactoring them, but I'm afraid you're wrong in one case (e.g. once you hit a polygon you say you've hit something, and when you draw a polygon you still have to draw other polygons).
Your code may work correctly, but IMHO i's hard to know because the bool? 3 values (true, false, null) are warped into too abstract concepts to be understood easily. Especially does true mean "it's hit!" or "it's visible!"?
I suggest an enum WorldAction with 3 values like (visible, invisible and hit).

bool workAcrossWorlds(WorldAction Function(double shift) work) {
    if (work(0) == WorldAction.hit) {
      return true;
    }

    if (worldWidth == 0) return false;
    for (double shift = -worldWidth;; shift -= worldWidth) {
      final isHit = work(shift);
      if (isHit == WorldAction.invisible) break;
      if (isHit == WorldAction.visible) continue;
      return true;
    }
    for (double shift = worldWidth;; shift += worldWidth) {
      final isHit = work(shift);
      if (isHit == WorldAction.invisible) break;
      if (isHit == WorldAction.visible) continue;
      return true;
    }

    return false;
  }

[JaffaKetchup]

@monsieurtanuki Ok, I've made some more changes, lmk what you think.

fd129c2 adds your enum idea, which I think makes sense.

8903fc1 is a little bit more! I can revert it if you think it's too much. In order to avoid declaring the 2 properties in the 3 layers, you have to introduce an intermediary class which mixes in both mixins. So I've done that. Then, I've also made it so that the hit testing in the PolylineLayer & PolygonLayer don't need to worry about using workAcrossWorlds or the new enum, it's all handled behind the scenes. CircleLayer doesn't do this, hopefully the implementation makes it clear why. It still just mixes in the 2 mixins and declares the 2 properties.

[monsieurtanuki]

Hi @JaffaKetchup!

The new enum is good.

I wouldn't be that enthusiastic about the mixin tango, but again, I'm not fluent in those dart/flutter extended OOP features. I assume though that if in Java and C++ those features weren't implemented it was perhaps also for code clarity.
I'm a bit lost in the latest code, as I don't see how CirclePainter, PolygonPainter and PolylinePainter shouldn't extend the same class, as obviously they all need fields like MapCamera and LayerHitNotifier<R>?.
That said, as long as it works, why not.

The naming of InteractiveMultiWorldProjectableFeatureLayerPainter is "rather large".

I'm not convinced removing parameter required LatLng coordinate from method bool elementHitTest is such a good idea, as there may be cases where computing from/to LatLng returns different (or truncated) results.

TL;DR I agree with fd129c2 and I would be careful with 8903fc1.

[JaffaKetchup]

Ok, makes sense. I've reverted the second PR and just made a couple small changes that have no real effect but are just nice.

Thanks :)