Fix anchor calculation for line-center placement when the anchor is very near to line segment endpoints

[zmiao]

Launch Checklist

When using line-center placement mode, if the anchor point is very close to the endpoint of the line segment, its x or y coordinate may be exactly the same as the endpoints after rounding. In this case, we will have to update the anchor point to be equal to the endpoint. Otherwise, the anchor point will not be on the line, further leading to angle calculation error when projecting symbols.

• briefly describe the changes in this PR
• include before/after visuals or gifs if this PR includes visual changes

Before	After

• write tests for all new functionality

Before	After

• apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
• add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Fix anchor calculation for line-center placement when the anchor is very near to line segment endpoints</changelog>

[rreusser]

Makes sense and seems reasonable! 👍 I don't have definitely required changes and only wanted to check on a couple things before giving it my full 👍 👍

The first is the 0.01/0.99 threshold, which seems slightly arbitrary and segment-length-dependent when maybe scaling by the length could fix the scale of the adjustment to a single tile-pixel unit.

The second concern is a vague (and certainly slightly under-informed) feeling that something downstream seems slightly fishy if the angle of text depends on the relative position to the closest endpoint of the segment rather than the farthest endpoint (which, if I understand correctly, is how the angle gets snapped to vertical). It sounds though like downstream, during projection, the anchor is falling onto the wrong segment when this issue occurs, which then causes the angle to depend on two very nearby points? If so, then this makes total sense.

Despite my questions though, very nice tests and very nice catch+fix! 👏

[rreusser]

Are the 0.01/0.99 thresholds general? Would dependence on the length of the line either too often apply this for long lines or not apply this for short lines?

I tried scaling by the length of the line and seeing if it was within a pixel of either endpoint:

if (t * segmentDistance > segmentDistance - 1 && (anchor.x === b.x || anchor.y === b.y)) {
  ...
} else if (t * segmentDistance < 1 && (anchor.x === a.x || anchor.y === a.y)) {
  ...
} 

and it seemed to work, but I'm not certain this is actually better or more general.

[zmiao]

I am thinking t * segmentDistance > segmentDistance - 1 maybe two strong, the same as for 0.01/009. I took them to only indicate which point will the anchor be closed to. So I wander should we just use 0.5 as the loose comparison, the further anchor point comparison will anyway do the enhanced check. wdyt?

[rreusser]

Ah, I see! My main concern was that the 0.01/0.99 threshold might lead to false positives/negatives in particular, I think, when the two points are close together. Just to be certain, would 0.5 make it possible for a perfectly horizontal line to pass the check incorrectly? i.e. anchor.y === b.y when the points are not actually close together?

[rreusser]

I think almost all of my remaining confusion comes down to this single condition:

anchor.x === b.x || anchor.y === b.y

Would a perfectly horizontal line (anchor.y = b.y) pass this condition when the two points are not actually close together?

[zmiao]

nice point! The same for perfectly vertical line. Since the anchor is calculated as (vectorB - vectorA) * t, which is [b.x-a.x, b.y - a.y] * t => [dx, dy] * t
so can we use following check, which is inspried from your solution:

if (dx !== 0 && dy !== 0) // exclude perfectly vertical/horizontal case {
    if ((dx * t < 1 || dy * t < 1) && (anchor.x === a.x || anchor.y ==== a.y) ) { 
        anchor.x = a.x;
        anchor.y = a.y;
    } else if ((dx * t > dx - 1 || dy * t > dy - 1) && (anchor.x === b.x || anchor.y ==== b.y) ) { 
        anchor.x = b.x;
        anchor.y = b.y;
    }
}

[zmiao]

@rreusser, thanks to @mpulkki-mapbox 's suggestion, I think the best option would be just removing the rounding process, and keep the resolution of the anchor as it is.

[rreusser]

That seems like a nice solution! I don't think there are generally thousands of characters, so it's hard to imagine that upgrading a couple numbers from two bytes to four would have a detrimental effect on performance. Sorry to drag my heels on this one, but it seemed tricky enough that I wanted to make sure the motivation and behavior was really clear. 🙇 This seems like a reasonable solution. 👍

[rreusser]

In particular, just to check my understanding (since I'm also considering how this will interact with #10755), this must not be the line at which it's computing the angle, since this depends on line points which should be plenty separated:

mapbox-gl-js/src/symbol/projection.js

Line 492 in 63380e7

const segmentAngle = angle + Math.atan2(current.y - prev.y, current.x - prev.x);

[zmiao]

In particular, just to check my understanding (since I'm also considering how this will interact with #10755), this must not be the line at which it's computing the angle, since this depends on line points which should be plenty separated:

mapbox-gl-js/src/symbol/projection.js

Line 492 in 63380e7

const segmentAngle = angle + Math.atan2(current.y - prev.y, current.x - prev.x);

@rreusser , I draw a simple picture to show this special case:

The a, b points are endpoints on the line segment when we calculating the anchors. After rounding the anchor is shifted from the line with 1 unit in the y-direction. When we do the symbol projection, we will firstly assign current = prev = projectedAnchor, after processing, we will then get current = projectB, prev = projectedAnchor. current and prve then will be taken into calculating the segmentAngle, which leads to the wrong angle.

[zmiao]

@rreusser thank you for the comments, for your concerns:

The first is the 0.01/0.99 threshold, which seems slightly arbitrary and segment-length-dependent when maybe scaling by the length could fix the scale of the adjustment to a single tile-pixel unit.

I agree, I pick them to imply that the anchor is nearer to either start point a or endpoint b, I think using 0.5 is enough. Since we have an enhanced check of (anchor.x === b.x || anchor.y === b.y) / (anchor.x === a.x || anchor.y === a.y), wdyt?

The second concern is a vague (and certainly slightly under-informed) feeling that something downstream seems slightly fishy if the angle of text depends on the relative position to the closest endpoint of the segment rather than the farthest endpoint (which, if I understand correctly, is how the angle gets snapped to vertical). It sounds though like downstream, during projection, the anchor is falling onto the wrong segment when this issue occurs, which then causes the angle to depend on two very nearby points? If so, then this makes total sense.

As depicted in the picture of my last comment, the anchor is used for the calculation of the point.

[rreusser]

This seems like a pretty reasonable solution. 👍 👏