Skip to content
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.

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

arrow sweep #1740

Merged
merged 10 commits into from
Jul 9, 2023
Merged

arrow sweep #1740

merged 10 commits into from
Jul 9, 2023

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jul 8, 2023

Introduces the new sweep option for the arrow mark, that allows to ensure that all arrows bulge in the same direction.

Also ensures that no arrowhead is drawn if headLength = 0 (as indicated in the comment in src/marks/arrow).

TODO:

  • wording? sweep could be something else, order-x could be x, order-y could be y, and order could be xy… or something else.
  • test the horizontal case
  • fix bug with the arrowhead when the sweep flag is flipped
  • document

closes #1739

@Fil Fil requested a review from mbostock July 8, 2023 15:29
@Fil Fil mentioned this pull request Jul 8, 2023
@Fil Fil marked this pull request as ready for review July 8, 2023 16:50
this.sweep == null
? 1
: this.sweep === "order-x"
? descending(x1, x2)
Copy link
Member

Choose a reason for hiding this comment

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

This will return 0 when x1 == x2, which will result in a bend angle of 0 below, which I don’t think is correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The use case I had in mind for order-x is when y is constant; in that case this test would be for links of length 0, and bending can be 0.

However I hadn't really given much thought to the fact that y can be variable. In that case, I don't really know what the user intent might be by specifying order-x (instead of order, which always “bends” unless the link has length 0). The current code, which makes a straight line when the order is a tie on x, doesn't look wrong and might even be useful (?):

Plot.arrow([-2, -1, 0, 1, 2], {
  x1: Plot.identity,
  x2: (d) => d * 1.1,
  y1: 0,
  y2: 1,
  sweep: "order-x",
  bend: -20
}).plot()

Copy link
Member

Choose a reason for hiding this comment

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

I think it’s okay if you document it.

Copy link
Member

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Per 5e313d9.

docs/marks/arrow.md Outdated Show resolved Hide resolved
src/marks/arrow.d.ts Outdated Show resolved Hide resolved
src/marks/arrow.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

mbostock commented Jul 9, 2023

If you merge #1741, this looks good to me.

(I also thought about renaming the tests to group the arc diagrams together, instead of making the dataset the primary aspect of the test name… but I’m not sure it matters. The test names should articulate what they are trying to achieve, and I think “arc diagram” is sufficient here. Though “arcs” would also suffice.)

@Fil Fil enabled auto-merge (squash) July 9, 2023 15:51
@Fil Fil merged commit e44dd3d into main Jul 9, 2023
@Fil Fil deleted the fil/arrow-sweep branch July 9, 2023 16:50
Fil added a commit that referenced this pull request Aug 21, 2023
* arrow sweep option; note that I also removed the arrow head if headLength is zero.
* miserables.json
* miserables arc diagram
* the arrow head and insets computations depend on the flipped bend angle
* darker initializer
* functional sweep (#1741)
* ±[xy]

---------

Co-authored-by: Mike Bostock <[email protected]>
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* arrow sweep option; note that I also removed the arrow head if headLength is zero.
* miserables.json
* miserables arc diagram
* the arrow head and insets computations depend on the flipped bend angle
* darker initializer
* functional sweep (observablehq#1741)
* ±[xy]

---------

Co-authored-by: Mike Bostock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arrow sweep strategy
2 participants