Skip to content

fix markers on lines with variable aesthetics #2094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 28, 2024
Merged

fix markers on lines with variable aesthetics #2094

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a0176b
for lines with variable aesthetics we want to maintain the higher-lev…
Fil Jun 17, 2024
e7f25c9
better marker strategy (#2095)
mbostock Jun 26, 2024
6fffceb
Merge branch 'main' into fil/group-markers
mbostock Jul 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 27 additions & 8 deletions src/marker.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import {select, selectAll, group} from "d3";
import {create} from "./context.js";

export function markers(mark, {marker, markerStart = marker, markerMid = marker, markerEnd = marker} = {}) {
Expand Down Expand Up @@ -100,16 +101,15 @@ function markerTick(orient) {
let nextMarkerId = 0;

export function applyMarkers(path, mark, {stroke: S}, context) {
return applyMarkersColor(path, mark, S && ((i) => S[i]), context);
return applyMarkersColor(path, mark, S && ((i) => S[i]), null, context);
}

export function applyGroupedMarkers(path, mark, {stroke: S}, context) {
return applyMarkersColor(path, mark, S && (([i]) => S[i]), context);
export function applyGroupedMarkers(path, mark, {stroke: S, z: Z}, context) {
return applyMarkersColor(path, mark, S && (([i]) => S[i]), Z, context);
}

function applyMarkersColor(path, {markerStart, markerMid, markerEnd, stroke}, strokeof = () => stroke, context) {
function applyMarkersColor(path, {markerStart, markerMid, markerEnd, stroke}, strokeof = () => stroke, Z, context) {
const iriByMarkerColor = new Map();

function applyMarker(marker) {
return function (i) {
const color = strokeof(i);
Expand All @@ -126,7 +126,26 @@ function applyMarkersColor(path, {markerStart, markerMid, markerEnd, stroke}, st
};
}

if (markerStart) path.attr("marker-start", applyMarker(markerStart));
if (markerMid) path.attr("marker-mid", applyMarker(markerMid));
if (markerEnd) path.attr("marker-end", applyMarker(markerEnd));
if (!(markerStart || markerMid || markerEnd)) return;

const start = markerStart && applyMarker(markerStart);
const mid = markerMid && applyMarker(markerMid);
const end = markerEnd && applyMarker(markerEnd);
if (Z) {
for (const g of group(
path.filter((i) => i.length >= 2),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
   }
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Capture d’écran 2024-06-18 à 08 39 29

With your patch it shows as I expect:
Capture d’écran 2024-06-18 à 08 44 06

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(d) => Z[select(d).datum()[0]]
).values()) {
if (start) select(g.at(0)).attr("marker-start", start);
if (mid) {
selectAll(g.slice(1)).attr("marker-start", mid);
selectAll(g).attr("marker-mid", mid);
}
if (end) select(g.at(-1)).attr("marker-end", end);
}
} else {
if (start) path.attr("marker-start", start);
if (mid) path.attr("marker-mid", mid);
if (end) path.attr("marker-end", end);
}
}
58 changes: 58 additions & 0 deletions test/output/groupMarker.svg
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading