fix markers on lines with variable aesthetics

[Fil]

for lines with variable aesthetics we want to maintain the higher-level semantics of markers:

• markerStart matches the start of the line
• markerMid matches the points which are not at the start or the end
• markerEnd matches the end of the line

Since these lines are implemented as multiple paths, we need to change the low-level implementation of markers:

• markerStart only applies to the first segment of a line
• markerMid applies to all the segments, complemented by the start of all but the first segments
• markerEnd only applies to the last segment of a line

closes #2093

[mbostock]

Functionally this seems correct, but I expect we could more efficiently implement it by augmenting applyMarker, rather than re-grouping the data and re-selecting the elements. The paths are already grouped, so it feels funny to redo all the work to group the data again.

[Fil]

I wonder if segments of length 1 are not a bug already—independently of the grouping?

[mbostock]

Could be. The only case I noticed was at the end of a line, which seems unavoidable. Are there others?

[Fil]

No, I think that's the only case. It makes "broken arrows" (if we apply a marker-end: arrow), which is why we have to filter it out here. I'll investigate.

[mbostock]

Here’s a patch which “fixes” it. But I think it’s valid for the last segment to have a single index if it has different aesthetics. It might be visible in the case where the stroke-linecap is round or square.

diff --git a/src/style.js b/src/style.js
index a1af100f..420d7bd3 100644
--- a/src/style.js
+++ b/src/style.js
@@ -261,6 +261,7 @@ export function* groupIndex(I, position, mark, channels) {
   for (const G of Z ? groupZ(I, Z, z) : [I]) {
     let Ag; // the A-values (aesthetics) of the current group, if any
     let Gg; // the current group index (a subset of G, and I), if any
+    let singleton = false; // does this series only have one group?
     out: for (const i of G) {
       // If any channel has an undefined value for this index, skip it.
       for (const c of C) {
@@ -273,8 +274,8 @@ export function* groupIndex(I, position, mark, channels) {
       // Otherwise, if this is a new group, record the aesthetics for this
       // group. Yield the current group and start a new one.
       if (Ag === undefined) {
-        if (Gg) yield Gg;
         (Ag = A.map((c) => keyof(c[i]))), (Gg = [i]);
+        singleton = true;
         continue;
       }
 
@@ -287,13 +288,15 @@ export function* groupIndex(I, position, mark, channels) {
         if (k !== Ag[j]) {
           yield Gg;
           (Ag = A.map((c) => keyof(c[i]))), (Gg = [i]);
+          singleton = false;
           continue out;
         }
       }
     }
 
-    // Yield the current group, if any.
-    if (Gg) yield Gg;
+    // Yield the current group, if any. For non-singleton series, the last group
+    // might contain only a single index; this is ignored.
+    if (Gg?.length > 1 || singleton) yield Gg;
   }
 }
 

[Fil]

When the last segment has only one point the linecap has the default ("horizontal") orientation, which makes it feel broken (here, shown with transparency so we can see better):

With your patch it shows as I expect:

[Fil]

I think I want to apply this patch! Not strictly necessary for this PR, but it seems reasonable to do it, and it will clean up the awkward tests on I.length > 1

[mbostock]

I’m not convinced we want the patch… Let’s land this without it and revisit if needed.