Reworked markers

[ignatz]

@ignatz:
None of the individual changes is very critical. I'm not sure they're worth the API-breaking change. Maybe simplification + increase consistency can justify it. I can be swayed either way. Let me know what you think.

---

@JaffaKetchup:

• No more builders: now we use Marker.child for extra efficiency
• Marker's constructor is now constant, along with MarkerLayer
• rotateOrigin and rotateAlignment have been removed, as not sure of their valid use-case
• Anchor, AnchorPos, and all anchor terminology have been removed, to simplify
• Only relative Alignments now supported, but now also non pre-provided ones
• Resolved confusion around meaning of anchors/position/alignment/relativity

Quite the breaking change!

[JaffaKetchup]

I'm up for the renaming to alignment, but definitely not AnchorAlignment. In this case, it could be considered that the anchor is opposite to the alignment, so putting them together is very confusing.

I'm also up for most of the simplification, but not necessarily the split of Marker into two constructors. I'm a little confused by the rest of it, an explanation would be good, as I can't make sense of the diff :D (it looks like you may have removed the functionality of a custom anchor position?)

Is there a way to make this less breaking but also simplified?

[ignatz]

I'm up for the renaming to alignment, but definitely not AnchorAlignment. In this case, it could be considered that the anchor is opposite to the alignment, so putting them together is very confusing.

What do you mean by "the anchor could be opposite to the alignment"?

I'm also up for most of the simplification, but not necessarily the split of Marker into two constructors. I'm a little confused by the rest of it, an explanation would be good, as I can't make sense of the diff :D (it looks like you may have removed the functionality of a custom anchor position?)

No functionality should have been removed. Let me try to expand: Marker has for the longest time received an Anchor, which used to be given in absolute coordinates. Then AnchorPos was introduced to capture both old-style absolute Anchors as well as relative aligned Anchors (e.g. center-left). In the relative case, it was the marker constructor that given the absolute marker wxh would calculate the absolute Anchor. This led to the Marker's constructor to no longer be const and AnchorPos trying to emulate tagged-unions, which Dart doesn't support.

Rather than multiplexing on the AnchorPos level, I changed it to two different constructors, which allows the center-aligned and absolute Anchor paths to remain const constructed and gets rid of the pseudo tagged union.

In other words: previously it was AnchorPos that had two constructors now it's Marker 🤷‍♀️

[JaffaKetchup]

What do you mean by "the anchor could be opposite to the alignment"?

Well, if the anchor is at the bottom, the item is 'aligned' to the top of it's box, IYSWIM.

Ok, I get the point of two seperate constructors to enable const, but I'm not sure it's worth it. I do agree with trying to remove the whole AnchorPos trying to represent a union though. Is there a middle ground somewhere?

[ignatz]

Well, if the anchor is at the bottom, the item is 'aligned' to the top of it's box, IYSWIM.

I see now. I guess, I've only ever used anchors. Maybe MarkerAlignment then? Open to any name

Ok, I get the point of two seperate constructors to enable const, but I'm not sure it's worth it. I do agree with trying to remove the whole AnchorPos trying to represent a union though. Is there a middle ground somewhere?

I'm not sure either. Take this as an RFC. As a anchor user, I was a bit surprised by this extra indirection and pseudo tagged union.

Anyway, as for alternatives:

• You either have to multiplex somewhere (Previously AnchorPos, now Marker)
• Or ask the user to convert to an Anchor themselves, i.e.

Marker(
  width: 30,
  height: 30,
  anchor: Anchor.fromAlignment(topLeft, 30, 30),
  /*...*/
)

[JaffaKetchup]

Maybe some opinions from the other guys would be helpful :)

MarkerAlignment

We just need to be careful here. Currently, I think only 'anchor' makes sense - we'd need to invert the behaviour if we start calling it 'alignment'.

[TesteurManiak]

Sorry for the late comment, I agree with @JaffaKetchup here, "anchor" in my opinion makes more sense and I see it as different from an "alignment" property (and it feels weird to have a constructor AnchorAlignment.align) but I might be biased because it's the term I've been used to for years.

Other than the name I'm ok with the other changes you've brought.

[ignatz]

Thanks for chiming in. I'm not very partial to any names and happy to adopt any suggestion, I'm just a bit lost in the conversation.

Currently we have the following names (starting from Marker):

• Marker(Anchor anchor, ...)
• Marker.align(AnchorAlignment alignment, ...)

which names would you like to have changed?

My original suggestion was to rename AnchorAlignment so that Marker.align would receive a correspondingly named MarkerAlignment instead 🤷‍♀️ . Just let me know and I'll change it in a heartbeat. Thanks

[TesteurManiak]

Thanks for the quick reply! I was referring to AnchorPos being renamed AnchorAlignment and related properties being renamed from anchorPos to alignment.

[ignatz]

Sorry, I'm a bit slow today. Putting your two responses together:

"anchor" in my opinion makes more sense

I was referring to AnchorPos being renamed AnchorAlignment and related properties being renamed from anchorPos to alignment

Are you saying that you're happy with the current state of naming with "AnchorPos" out of the picture in favor of AnchorAlignment?

[TesteurManiak]

Sorry, I'm a bit slow today. Putting your two responses together:

Don't worry 😆

Are you saying that you're happy with the current state of naming with "AnchorPos" out of the picture in favor of AnchorAlignment?

No, I was actually saying that I prefer the old AnchorPos naming.

[ignatz]

I totally nailed that 50:50 👼 .

I did replace all the "Alignment" with "Pos/position". PTAL

[TesteurManiak]

LGTM 👍

[JaffaKetchup]

Sorry to tread on an approval, but I think we've missed something major here. There's now very little point to having AnchorPos at all. It's just a container for an alignment and text direction. If we're happy to forfeit the text direction - which I would be happy to, considering that this doesn't handle text and the position of an anchor shouldn't be affected by text direction - then we can remove AnchorPos entirely.

I'm also still not totally satisfied with the naming. position isn't descriptive enough for me, given that it has exactly the same issues as alignment - nothing is identifying what is in relation to what.

Take a look at the image below:

Look at the marker just below London. I would say that its anchor position is London, and therefore top-center relative to the marker. I would say that the marker position is bottom-center relative to the anchor. The buttons at the top left change the anchor position, previously called anchorPos, currently called position. However, they are not actually changing the anchor position - they are changing the marker position. We cannot mix and match position and AnchorPos and alignment without qualifying what they are all in relation to. For example, this PR treats anchor and position as equivalent, even though they would appear to refer to completely different things.

However, I'm assuming what anchor means here. I'm assuming that it means center of rotation, which makes sense in my head. But, according to the documentation which doesn't make sense, it means something different: the position of the widget in relation to the normal center. And of course, I haven't at all factored in rotateAlignment or rotateOrigin - I'm not even sure what the point in those is.

TLDR: the naming still isn't right, and everything is still self-contradictory. This includes the current code base.

[JaffaKetchup]

@TesteurManiak @ignatz What do you think about this? Things can be added back as necessary, but this is as clean as possible, for a starting base:

• There's nothing for rotateOrigin and rotateAlignment anymore, as I can't think why they would be needed, but they can be added back if necessary.
• There's no more absolute/exact units, but on the other hand, any Alignment is now valid - I've replaced the switch statement which resolved it all with some basic formulas - so that should be less of an issue. It also means we can remove Anchor and AnchorPos entirely.
• There is now no reference to anchor, instead alignment is used, and the documentation clearly states what that is in relation to (relative to the normal center at point).
• Calculation of the left, top, etc. of each marker is now again done at build time, instead of construction. I'm aware that this is less performant, but it allows for const Markers, and I'm not sure how actually less performant it is.

import 'dart:math';

import 'package:collection/collection.dart';
import 'package:flutter/widgets.dart';
import 'package:flutter_map/src/map/camera/camera.dart';
import 'package:flutter_map/src/misc/point_extensions.dart';
import 'package:flutter_map/src/misc/private/bounds.dart';
import 'package:latlong2/latlong.dart';

/// Represents a coordinate point on the map with an attached widget [builder],
/// rendered by [MarkerLayer]
///
/// Some properties defaults will absorb the values from the parent [MarkerLayer],
/// if the reflected properties are defined there.
@immutable
class Marker {
  final Key? key;

  /// Coordinates of the marker
  ///
  /// This will be the center of the marker, assuming that [alignment] is
  /// [Alignment.center] (default).
  final LatLng point;

  /// Returns the marker widget itself
  final WidgetBuilder builder;

  /// Bounding box width of the marker
  final double width;

  /// Bounding box height of the marker
  final double height;

  /// Alignment of the marker relative to the normal center at [point]
  ///
  /// For example, [Alignment.topCenter] will mean the entire marker widget is
  /// located above the [point].
  ///
  /// The center of rotation will be opposite this.
  ///
  /// Defaults to [Alignment.center] if also unset by [MarkerLayer].
  final Alignment? alignment;

  /// Whether to counter rotate markers to the map's rotation, to keep a fixed
  /// orientation
  ///
  /// When `true`, markers will always appear upright and vertical from the
  /// user's perspective. Defaults to `false` if also unset by [MarkerLayer].
  ///
  /// Note that this is not used to apply a custom rotation in degrees to the
  /// marker. Use a widget inside [builder] to perform this.
  final bool? rotate;

  const Marker({
    this.key,
    required this.point,
    required this.builder,
    this.width = 30,
    this.height = 30,
    this.alignment,
    this.rotate,
  });
}

@immutable
class MarkerLayer extends StatelessWidget {
  final List<Marker> markers;

  final Alignment alignment;

  final bool rotate;

  const MarkerLayer({
    super.key,
    this.markers = const [],
    this.alignment = Alignment.center,
    this.rotate = false,
  });

  @override
  Widget build(BuildContext context) {
    final map = MapCamera.of(context);

    return Stack(
      children: markers
          .map((m) {
            // Resolve real alignment
            final left = 0.5 * m.width * ((m.alignment ?? alignment).x + 1);
            final top = 0.5 * m.height * ((m.alignment ?? alignment).y + 1);
            final right = m.width - left;
            final bottom = m.height - top;

            // Perform projection
            final pxPoint = map.project(m.point);

            // Cull if out of bounds
            if (!map.pixelBounds.containsPartialBounds(
              Bounds(
                Point(pxPoint.x + left, pxPoint.y - bottom),
                Point(pxPoint.x - right, pxPoint.y + top),
              ),
            )) return null;

            // Apply map camera to marker position
            final pos = pxPoint.subtract(map.pixelOrigin);

            return Positioned(
              key: m.key,
              width: m.width,
              height: m.height,
              left: pos.x - right,
              top: pos.y - bottom,
              child: (m.rotate ?? rotate)
                  ? Transform.rotate(
                      angle: -map.rotationRad,
                      alignment: (m.alignment ?? alignment) * -1,
                      child: m.builder(context),
                    )
                  : m.builder(context),
            );
          })
          .whereNotNull()
          .toList(),
    );
  }
}

[ignatz]

Sorry to tread on an approval, but I think we've missed something major here. There's now very little point to having AnchorPos at all. It's just a container for an alignment and text direction. If we're happy to forfeit the text direction - which I would be happy to, considering that this doesn't handle text and the position of an anchor shouldn't be affected by text direction - then we can remove AnchorPos entirely.

I agree with this assessment. The underlying issues, i.e. two opposing ways, of achieving the same predates my change. Plus:

• is alignment aligning the marker relative to the anchor or the anchor relative to the marker?
• no clue why we added text direction in the past 🤷‍♀️

Thanks for taking the time to make a counter proposal. I like that it solves the issue of having two ways of achieving the same goal. I also like that it removes the arbitrary restriction of only supporting cardinal alignments, i.e. alignment is now just as flexible as anchor just with relative rather than absolute offsets, which likely is also easier to use. Arguably what aligns to what is still somewhat arbitrary but the docstring makes that very clear. I'd even remove the point on center of rotation. As you said as well, it's not clear to me why you'd ever rotate around anything other than the anchor point 🤷‍♀️ .

Really nothing to critique. Ship it.

[JaffaKetchup]

no clue why we added text direction in the past

I added it to properly support resolution of AlignmentGeometry, given that it's usually better to accept that instead of Alignment. However, for the complexity it now adds (with it being the sole requirement for a whole new object), I would get rid of it.

---

One thing that we perhaps forgot is that once a builder is defined, the call of the Marker constructor cannot be const, and so what is even the point of having it allowed to be const at all :D - bit of an oversight on all our parts. Much better to cram as much precalculation in there as possible, which is what I'll do.

This proposal also means that there is no behaviour change, even though it looks like there should be. This might be a little difficult to explain concisely. I won't add deprecations, as it would require keeping Anchor and AnchorPos for the sake of it, which I'd like to begone.

[ignatz]

One thing that we perhaps forgot is that once a builder is defined, the call of the Marker constructor cannot be const, and so what is even the point of having it allowed to be const at all :D - bit of an oversight on all our parts. Much better to cram as much precalculation in there as possible, which is what I'll do.

It's not clear to me why Marker having a builder function would make Markers impossible to cons-construct. Clearly we did it before.
Since you're opening that can of worms, it was never quite clear to me why Marker even expects a build function by default. It's more efficient for partial tree rebuilds if the marker widget remains the same. Also the builder functoin doesn't add anything, since you can always employ a Builder widget. It would love for the marker to simply receive a child widget. Today, if you want to minimize rebuilds you have to write silly code:

  Widget build(BuildContext context) {
     final marker = ColoredBox(color: Colors.pink);
     return MarkerLayer(
        markers: [
           Marker(builder: (_) => marker),
        ],
     }
  }

I'd much prefer:

  Widget build(BuildContext context) {
     return MarkerLayer(
        markers: [
           Marker(child: ColoredBox(color: Colors.pink)),

           // Or if you depend on the build context
           Marker(child: Builder(builder: (BuildContext context) => foo(context))),
        ],
     }
  }

[JaffaKetchup]

Clearly we did it before.

I don't think we did. The definition might have been const, but (correct me if I'm wrong), that doesn't mean anything unless the invocation actually uses const. And a function is always non-const. It has always been used as Marker(), not const Marker().

It would love for the marker to simply receive a child widget. Today, if you want to minimize rebuilds you have to write silly code:

I'm also not sure why it takes a builder. Is there anything stopping us from allowing both? And also, question, are rebuilds of child minimized if Marker() is not called constantly? If it needs to be const, we could create a seperate constructor.

[JaffaKetchup]

Also, I'm not sure there's a difference by calculating the left and top of the marker in the Marker constructor. Is it better to potentially save microseconds here at scale (reduces number of computations at build time), and not have const constructor (bearing in mind the above with potential for const with child), or the other way around?

[ignatz]

I'm also not sure why it takes a builder. Is there anything stopping us from allowing both? And also, question, are rebuilds of child minimized if Marker() is not called constantly? If it needs to be const, we could create a seperate constructor.

It doesn't need to be const. For the tree-diffing to work effectively w/o providing an explicit key, the widget object id needs to remain stable, in which case it will simply skip calling the Widget's build again.

What happened to: "Let's have one way of doing things"? 🥁 🙃 We're already in the business of breaking API compatibility and we'd just be nudging it closer to the Flutter convention, i.e. just receive a child.

I don't think we did. The definition might have been const, but (correct me if I'm wrong), that doesn't mean anything unless the invocation actually uses const. And a function is always non-const. It has always been used as Marker(), not const Marker().

Maybe we're talking about different things but Marker used to have a const constructor and my change in its current form restores Marker to have a const constructor.

Also, I'm not sure there's a difference by calculating the left and top of the marker in the Marker constructor. Is it better to potentially save microseconds here at scale (reduces number of computations at build time), and not have const constructor (bearing in mind the above with potential for const with child), or the other way around?

These are efficient math ops, so we're talking ~3 orders of magnitude less, i.e. nanoseconds. That's an optimization you really won't notice. Especially if you compare it to the still required projection/trigonometry operations.

I did a quick micro-benchmark (never trust a micro-benchmark), which showed that const vs non-const construction of an otherwise virtually identical object makes virtually no difference. However, as soon as you add a constructor body it slows 10x.
In that spirit getting rid of the builder functor will certainly save more cycles than the few additions and multiplications.

[JaffaKetchup]

What happened to: "Let's have one way of doing things"? 🥁 🙃 We're already in the business of breaking API compatibility and we'd just be nudging it closer to the Flutter convention, i.e. just receive a child.

You're quite right, keeping me in check :D. One way = best way.

Maybe we're talking about different things but Marker used to have a const constructor and my change in its current form restores Marker to have a const constructor.

We're talking about similar but different things. USAGE of the Marker constructor has never been able to be const due to the builder. DEFINITION of the Marker constructor has sometimes been const, sometimes not.

Anyway, doesn't matter now!

These are efficient math ops, so we're talking ~3 orders of magnitude less, i.e. nanoseconds. That's an optimization you really won't notice. Especially if you compare it to the still required projection/trigonometry operations.

Yep, that's the conclusion I reached as well.

Significantly outdated

[JaffaKetchup]

Welp, all of those changes have been made. Quite the big change, but all for the best I think!
Let me know what you guys think.

[ignatz]

Let me know what you guys think.

I love it

[JaffaKetchup]

LGTM with feedback from @ignatz. Will try to wait for a second approval as a lot of the code is mine.

[ignatz]

LGTM with feedback from @ignatz. Will try to wait for a second approval as a lot of the code is mine.

And thanks for jumping in with a much better counter proposal 🙏

[TesteurManiak]

I came up with the same conclusions as @ignatz, I've compared the current implementation with this one and there seem to be no impact in performances despite the const constructor but had an impact when the builder property had a constructor body. (tested with Flutter Web on Chrome with canvaskit enabled)

Otherwise LGTM 👍

[Zverik]

So, the question now is: what is the best way to upgrade markers from fixed anchors to alignment? Right now I did this with magic contants (like, Alignment(0.6, 0.0) looks roughly the same), but for some variable-width markers that doesn't work.

[Zverik]

Update: for x, this amounted to 1.0 - 2 * d / width, where d is how many pixels from the left bound the anchor position is.

Maybe we should make a constructor for anchor position that translates into the default constructor?

[JaffaKetchup]

That sounds sensible, I've created #1771 as a feature request.

[Zverik]

Thanks JK, and sorry, I should've made the ticket myself instead of ranting :)

[Anyaoha]

Hello I'm curious. If anchorPos is completely removed. How then am I supposed to modify my code with this parameter.
Please help.

MarkerClusterLayerWidget(
            options: MarkerClusterLayerOptions(
              // Function to compute cluster marker size based on the number of markers
              computeSize: (markers) {
                var count = markers.length;
                var scaleFactor =
                    (log(count + 1) / log(widget.maxClusterSize + 1));
                var size = widget.minClusterSize +
                    scaleFactor *
                        (widget.maxClusterSize - widget.minClusterSize);
                return Size(size, size);
              },
              maxClusterRadius: widget.maxClusterRadius,
              fitBoundsOptions: FitBoundsOptions(
                padding: EdgeInsets.all(50),
              ),
              markers: convertedPlacesWithRefs
                  .map((pair) => Marker(
                        width: (widget.markerIcon as Icon).size! + 8,
                        height: (widget.markerIcon as Icon).size! + 8,
                        **anchorPos: AnchorPos.align(AnchorAlign**
                            .center), // marker: Marker.align(MarkerAlign.center) this changed prior to Flutter 3.19.1 so we may need to maintain the flutter_map and flutter_map_marker_cluster versions before the flutter version upgrade. Or just change/remove the parameter to the current parameters.
                        point: pair.coordinates,
                        builder: (ctx) => GestureDetector(
                          onTap: () {
                            FFAppState().selectedPlace = pair.documentRef;
                            if (widget.callBackAction != null) {
                              widget.callBackAction!();
                            }
                          },

The main code highlighted is

anchorPos: AnchorPos.align(AnchorAlign.center)

[JaffaKetchup]

Hi @Anyaoha,
Please check https://docs.fleaflet.dev/getting-started/migrating-to-v6#marker-layer to see if that helps, otherwise please get in touch on the Discord server :)