Voronoi initializer (for pointer interactions) #1623
Conversation
|
I couldn’t resist trying the Voronoi initializer idea… I dunno if we need to make similar changes to the Delaunay marks? I think maybe it doesn’t make sense there, though, because there’s a not a 1:1 correspondence between the data and the geometry. Screen.Recording.2023-05-23.at.7.01.59.PM.mov |
|
the voronoi cell now depends on i and fi (with a test showing facet: exclude) |
Fil
changed the title
There was a problem hiding this comment.
This seems mostly fine… but there’s something that bothers me. I guess it’s that we need to create nested/faceted channels because we can’t assume that the facets are exclusive. #1648 And our workaround abuses how channels are intended to be represented (parallel to the data). And it’s especially funny that the Voronoi mark handles this but the stack transform doesn’t—the stack transform is far more frequently used!
I would rather fix the non-exclusive facet problem generally rather than apply the workaround in this PR. I think we probably need a redux of the approach in #1069. Ideally, it’s like a transform wrapper that says “this transform/initializer needs exclusive facets”, and it expands the faceted index and channels as needed to meet the constraint that the facets are exclusive, and the downstream code therefore doesn’t need any changing.
Fil
force-pushed
the
fil/voronoi-initializer
branch
2 times, most recently
from
4996723
to
3792cc5
Compare
Fil
force-pushed
the
fil/voronoi-initializer
branch
from
d4192ad
to
4a6ef93
Compare
|
|
|
|
|
|
|
I've updated the description of this PR. |
Fil
force-pushed
the
fil/voronoi-initializer
branch
from
0d2d77d
to
f6c344a
Compare
Fil
force-pushed
the
fil/voronoi-initializer
branch
from
2967384
to
95f9f1c
Compare
| // TODO Accept other types of clips (paths, urls, x, y, other marks…)? | ||
| // https://github.com/observablehq/plot/issues/181 | ||
| export function maybeClip(clip) { | ||
| if (clip === true) clip = "frame"; | ||
| else if (clip === false) clip = null; | ||
| else if (clip != null) clip = keyword(clip, "clip", ["frame", "sphere"]); | ||
| return clip; | ||
| } |
There was a problem hiding this comment.
This doesn’t look used. Is this needed as part of this PR?
There was a problem hiding this comment.
Good catch. This was the result of a bad merge operation (this part belongs to #1624).
| [x, y] = maybeTuple(x, y); | ||
| return new DelaunayMark(data, {...options, x, y}); | ||
| return new DelaunayMark(data, {...basic({...options, x, y}, exclusiveFacets), initializer}); |
There was a problem hiding this comment.
Is exclusiveFacets only needed in the Voronoi case, not the general case of all Delaunay marks?
There was a problem hiding this comment.
Yes I think so. The hand-wavy "proof" is that the voronoi cells are "more" associated to each data point, and can be separated by facet after the transform, whereas the delaunay triangles (and mesh) are tied to the whole group of points, and can't be separated post-transform (any triangle that is not joining 3 points in the same facet would be inconsistent).
new description!
This PR chiefly addresses #1622, making the voronoi mark work with the tip option. A notable performance issue in the modified usStateCapitalsVoronoi test is that the clip path is created anew each time we rerender the mark (this is addressed separately, in #1624).
A secondary issue (#1858, making voronoi work with exclusive facets) was first addressed in an ad hoc fashion (in 71c49a2). After this review comment suggesting that this should be centralized, I reverted the change in 70f2352, then used the new exclusiveFacets transform (introduced in #1648) in 972558c et sq.
Closes #1622
Closes #1858.