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

spike vector shape + centroid transforms #1189

Merged
merged 16 commits into from
Dec 20, 2022
Merged

spike vector shape + centroid transforms #1189

merged 16 commits into from
Dec 20, 2022

Conversation

mbostock
Copy link
Member

@mbostock mbostock commented Dec 16, 2022

image

Plot.plot({
  width: 960,
  height: 600,
  projection: "albers-usa",
  length: {
    range: [0, 200]
  },
  marks: [
    Plot.geo(nation, {fill: "#e0e0e0"}),
    Plot.geo(statemesh, {stroke: "white"}),
    Plot.spike(counties.features, Plot.geoCentroid({stroke: "red", length: (d) => population.get(d.id)}))
  ]
})

TODO

  • Support the rotate option?
  • Support the anchor option?
  • Plot.spikeX and Plot.spikeY?

Fixes #1178. (The diffs to the tests are just the re-ordering of the aria-label and fill attributes.)

An alternate approach here would be to expose a “shape” option similar to how the dot mark allows you to specify a custom symbol. (The difference being that vector uses length and dot uses area, so you don’t have the confusion around the r sqrt scale.) That’d probably be more flexible and we could still do that in the future, but I thought having a builtin in spike mark would be nice—especially with the default fill and stroke.

@mbostock mbostock requested a review from Fil December 16, 2022 00:14
@Fil
Copy link
Contributor

Fil commented Dec 16, 2022

We need similar Plot.centroid for planar geometries (and when in doubt about the geometries, as a best guess).

A custom symbol would be nice, and hopefully shared with the other symbols (those associated with dot). This means two changes:

  1. we should rotate them with a svg transform, or alternatively make d3.path smart about rotate and translate.
  2. we need to pass a few configuration options (k for anchor, and width for the base width, and all the arrowHead stuff if we make the vector as configurable as Plot.arrow). The current API for D3's symbol only has path, size; we can probably add an optional {options} as a third parameter?

@mbostock mbostock changed the title spike mark + geoCentroid transform spike mark + centroid transforms Dec 17, 2022
@mbostock mbostock changed the title spike mark + centroid transforms spike vector shape + centroid transforms Dec 17, 2022
@mbostock
Copy link
Member Author

Okay, I have now:

  • Added a shape option to vector for arbitrary shapes (similar to dot’s symbol, but not a channel)
  • Added a spike shape, along with vector’s default arrow shape
  • Added a new centroid transform for planar centroids, along with the previous geoCentroid transform
  • Folded the spike mark into the existing vector mark; it is now only a convenience constructor
  • Added a nifty internal template helper for generating dynamic SVG transform attributes

I expect people will incorrectly apply the Plot.centroid transform to spherical geometry when they should be using the Plot.geoCentroid transform. I’m not sure what to do about that; I don’t think it’s possible for the centroid transform to detect that a projection is in use and throw an error or warning. And even if a projection is in use, we don’t know whether it’s a geographic/spherical projection or a planar transform anyway. But I think this is fine for now, and the difference is often imperceptible anyway, as in the spike map example.

Please take another look?

@Fil
Copy link
Contributor

Fil commented Dec 19, 2022

A slight difficulty when documenting, between width and radius. Could we settle on defining just one of them?

@mbostock
Copy link
Member Author

A slight difficulty when documenting, between width and radius.

You’re referring to the fact that there’s a width option for the vector mark, whereas a radius is passed to the vector mark’s custom shape? My thinking is that for the user, it’s more intuitive to specify a width (e.g., of the base of a spike) because that’s what you see; whereas for the shape implementor, it’s more intuitive to receive the radius (the offset from the anchor to the respect corners of the base) because that’s what you need to construct the geometry.

@Fil
Copy link
Contributor

Fil commented Dec 19, 2022

Yes. I understand the intent, but it means that the documentation for the custom shape object needs to explain that radius is half the width, which seems a bit verbose.

@mbostock
Copy link
Member Author

Would you prefer we use radius everywhere? Or r to match dot (even though it won’t support a channel)?

@Fil
Copy link
Contributor

Fil commented Dec 19, 2022

Any of these work equally. Maybe using r is indeed better since it's a bit more consistent with the rest.

README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

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

with the additional geometry option it looks like it's good to go

@Fil Fil added the geo Maps and projections label Dec 19, 2022
@Fil
Copy link
Contributor

Fil commented Dec 20, 2022

TIL! but we need to test centroid too :)

@mbostock
Copy link
Member Author

Added test to compare spherical centroids (red) and projected planar centroids (blue).

Screenshot 2022-12-20 at 10 42 20 AM

@mbostock
Copy link
Member Author

Also for reference here is the equirectangular centroid (green):

Screenshot 2022-12-20 at 10 48 05 AM

I guess it’s okay to lose this. Even though it’s “fast”, it’s only right if you’re using the equirectangular projection. Whereas the other two at least have a valid semantic meaning (either the center on the surface of the sphere, or the center on the projected plane).

@mbostock mbostock merged commit b31b8cc into main Dec 20, 2022
@mbostock mbostock deleted the mbostock/spike branch December 20, 2022 18:53
chaichontat pushed a commit to chaichontat/plot that referenced this pull request Jan 14, 2024
* spike, geoCentroid

* vector shape, template

* planar centroid

* document vector and spike

* document centroid and geoCentroid

* r instead of width

* Update README

* geometry option; update README

* lift size

* fancy splice

* projected centroid via initializer

* Update README

* Update README

* geoCentroid

* geoCentroid is faster, actually

* test centroids

Co-authored-by: Philippe Rivière <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
geo Maps and projections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Spike map
2 participants