add d3.trail() #168

wyhleonard · 2020-08-26T04:48:15Z

No description provided.

wyhleonard · 2020-08-26T05:24:02Z

This commit added the function as trail marks in Vega, which is similar to line marks, but can have variable width determined by backing data . Moreover, this commit also fixed the two gotchas described in the issue. Users can use d3.trail() like d3.line() to create the trail marks they want.
The first picture is produced by the method in Vega of trail marks, and the second picture is produced by the method in this commit.

Fil · 2020-08-26T07:39:54Z

Ah excellent! I was worried about this shape too https://observablehq.com/@fil/trail-marks ;-)

Another bit that would need fixing in this shape is when the data has more than 2 points—currently it draws one "loop" for each pair, with the consequence that the stroke is not the outline, and the shape has superpositions.

(You will also need to declare the function in src/index.js, show it in README.md and test it in tests/ for the PR to be complete.)

wyhleonard · 2020-08-26T08:22:06Z

Thank you for your suggestions, I will further improve this function with your suggestions. Actually, it's my first experience to contribute to open source libraries. So if i make some mistakes, please accept my most sincere apology.

wyhleonard · 2020-09-01T18:57:35Z

The second commit has improved the function of d3.trail(), including fixing the gotchas of superpositions and declaring the function in src/index.js, showing it in README.md and testing it in tests/trail-test.js. Now, d3.trail() can draw the trail marks as

However, there are still some problems in README.md. For example, it is unable to display pictures properly, and there is an unexpected slant in the text after Lines.

Fil · 2020-09-01T20:35:48Z

Wow this is beautiful!

(The images will display only when the PR is merged (they link to the master branch).)

curran · 2020-09-01T20:43:54Z

Someone should totally build an example with this that produces Minard's "Napoleon's March on Moscow" map.

curran · 2020-09-01T20:46:10Z

Also I wonder how close this gets us to being able to automatically produce shapes like this.

Fil · 2020-09-01T20:59:10Z

Two small issues:

Have you pushed all the code? To build this branch I had to add:

import array from "./array.js";

Secondly, the data from https://observablehq.com/@fil/trail-marks breaks it, because I've left a little 💣 with coincident points ( [300, 200, 0], [300, 200, 0.1] ).

wyhleonard · 2020-09-02T05:00:22Z

The third commit has solved some small issus in d3.trail(). Note that the point will be only drawed when its size is greater than 0.

Fil

Mostly style remarks to try and homogenize with the rest of the code base.

I'm wondering: it might be more performant if it did all the calls to x, y, size and defined only once, and saved the values in Float64Arrays. (Plus, you never know what accessors the user is going to call—or maybe someone is going to try size(random).)

src/point.js

src/trail.js

test/trail-test.js

README.md

wyhleonard · 2020-09-06T14:58:16Z

The fourth commit has completed the changes requested. Now, d3.trail() can draw a trail mark with coincident points as

Fil

Looks good to me!

If you would follow D3's general style conventions more closely (with spaces between operators and arguments, before curly brackets etc.) it will help do a cleaner merge. See https://gist.github.com/Fil/faa610218d355d927b19cf8e99d8dcb6

wyhleonard · 2020-09-07T01:18:32Z

Thank you for your reminder. I have followed D3's general style conventions in the fifth commit.

Fil · 2020-09-07T06:24:52Z

Thank you. Beautiful work! I think @mbostock and @jheer will be interested to review.

wyhleonard · 2020-09-07T06:55:40Z

It's my honor to contribute to D3. Thank you for your help again.

jheer · 2020-09-07T12:05:53Z

It's great to see someone improving the trail mark in Vega and adapting it to d3! Thanks!

I took a quick glance at the code, but didn't check the adapted logic for correctness. That said, I did notice some possible concerns:

Helper methods like intersectPoint and commonTangent generate object-valued outputs, possibly causing some performance overhead due to repeated object instantiation.
Inconsistencies in code style spacing. For example: else{ vs. else {, if( vs. if (, function(test){ vs. function(test) {.

wyhleonard · 2020-09-07T16:02:58Z

Thank you for your suggestions! The sixth commit has taken the two concerns above.

curran

Looks good! Added some minor nits.

I think the consensus is that the README should use let and const, while the implementation uses ES5.

curran · 2020-11-02T20:55:00Z

README.md

+When a trail is [generated](#_trail), the x accessor will be invoked for each [defined](#trail_defined) element in the input data array, being passed the element `d`, the index `i`, and the array `data` as three arguments. The default x accessor assumes that the input data are three-element arrays of numbers. If your data are in a different format, or if you wish to transform the data before rendering, then you should specify a custom accessor. For example:
+
+```js
+var data = [


Suggested change

var data = [

const data = [

curran · 2020-11-02T20:55:12Z

README.md

+  …
+];
+
+var trail = d3.trail()


Suggested change

var trail = d3.trail()

const trail = d3.trail()

curran · 2020-11-02T20:56:10Z

src/trail.js

+      return false;
+    }
+
+    // straddle experiment.


If it's an experiment, do we want to add it? Maybe remove this comment?

Suggested change

// straddle experiment.

// straddle.

curran · 2020-11-02T20:57:10Z

src/trail.js

+
+    // straddle experiment.
+    if (cross(x1 - x4, y1 - y4, x3 - x4, y3 - y4) * cross(x2 - x4, y2 - y4, x3 - x4, y3 - y4) > 0 ||
+       cross(x3 - x2, y3 - y2, x1 - x2, y1 - y2) * cross(x4 - x2, y4 - y2, x1 - x2, y1 - y2) > 0) {


Suggested change

cross(x3 - x2, y3 - y2, x1 - x2, y1 - y2) * cross(x4 - x2, y4 - y2, x1 - x2, y1 - y2) > 0) {

cross(x3 - x2, y3 - y2, x1 - x2, y1 - y2) * cross(x4 - x2, y4 - y2, x1 - x2, y1 - y2) > 0) {

Alignment reads well.

This is a lot for a single expression also. May be worth factoring out into variables.

curran · 2020-11-02T20:58:27Z

src/trail.js

+        x: x2,
+        y: y2
+      };
+    }


Suggested change

}

} else {

Seems odd to have else on a new line.

curran · 2020-11-02T21:00:15Z

src/trail.js

+        angle: alpha + beta - pi
+      };
+    }
+    else {


No need for this else, as the if returns. Can remove the brackets and un-indent here.

curran · 2020-11-02T21:01:21Z

src/trail.js

+
+    if (context == null) context = buffer = path();
+
+    var start,


Perhaps better to lift these declarations to the top, with the others.

wyhleonard · 2020-12-19T16:41:14Z

Thank you for your suggestions! The lastest commit has refined the codes and docs in d3-shape.

add d3.trail()

b9c0b9c

perfect d3.trail()

d8452da

solve the two small issues

8eb130b

Fil requested changes Sep 2, 2020

View reviewed changes

change-requested

dc9fc0b

Fil requested changes Sep 6, 2020

View reviewed changes

cleaner merge

6f9c047

Fil approved these changes Sep 7, 2020

View reviewed changes

the sixth commit

c7afae0

curran approved these changes Nov 2, 2020

View reviewed changes

refine codes and docs

fdaa556

Fil requested a review from mbostock December 19, 2020 16:42

Fil added the feature label Jan 23, 2021

Fil mentioned this pull request Apr 28, 2021

Adopt type=module #179

Merged

5 tasks

Fil mentioned this pull request May 1, 2021

trail marks? #164

Open

2 tasks

Fil mentioned this pull request Jan 5, 2022

Trails observablehq/plot#650

Open

mbostock removed the feature label May 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add d3.trail() #168

add d3.trail() #168

wyhleonard commented Aug 26, 2020

wyhleonard commented Aug 26, 2020

Fil commented Aug 26, 2020

wyhleonard commented Aug 26, 2020 •

edited

Loading

wyhleonard commented Sep 1, 2020 •

edited

Loading

Fil commented Sep 1, 2020

curran commented Sep 1, 2020

curran commented Sep 1, 2020

Fil commented Sep 1, 2020

wyhleonard commented Sep 2, 2020

Fil left a comment

wyhleonard commented Sep 6, 2020

Fil left a comment

wyhleonard commented Sep 7, 2020 •

edited

Loading

Fil commented Sep 7, 2020

wyhleonard commented Sep 7, 2020

jheer commented Sep 7, 2020

wyhleonard commented Sep 7, 2020

curran left a comment

curran Nov 2, 2020

curran Nov 2, 2020

curran Nov 2, 2020

curran Nov 2, 2020

curran Nov 2, 2020

curran Nov 2, 2020

curran Nov 2, 2020

wyhleonard commented Dec 19, 2020

	cross(x3 - x2, y3 - y2, x1 - x2, y1 - y2) * cross(x4 - x2, y4 - y2, x1 - x2, y1 - y2) > 0) {
	cross(x3 - x2, y3 - y2, x1 - x2, y1 - y2) * cross(x4 - x2, y4 - y2, x1 - x2, y1 - y2) > 0) {

add d3.trail() #168

Are you sure you want to change the base?

add d3.trail() #168

Conversation

wyhleonard commented Aug 26, 2020

wyhleonard commented Aug 26, 2020

Fil commented Aug 26, 2020

wyhleonard commented Aug 26, 2020 • edited Loading

wyhleonard commented Sep 1, 2020 • edited Loading

Fil commented Sep 1, 2020

curran commented Sep 1, 2020

curran commented Sep 1, 2020

Fil commented Sep 1, 2020

wyhleonard commented Sep 2, 2020

Fil left a comment

Choose a reason for hiding this comment

wyhleonard commented Sep 6, 2020

Fil left a comment

Choose a reason for hiding this comment

wyhleonard commented Sep 7, 2020 • edited Loading

Fil commented Sep 7, 2020

wyhleonard commented Sep 7, 2020

jheer commented Sep 7, 2020

wyhleonard commented Sep 7, 2020

curran left a comment

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

curran Nov 2, 2020

Choose a reason for hiding this comment

wyhleonard commented Dec 19, 2020

wyhleonard commented Aug 26, 2020 •

edited

Loading

wyhleonard commented Sep 1, 2020 •

edited

Loading

wyhleonard commented Sep 7, 2020 •

edited

Loading