plot icon indicating copy to clipboard operation
plot copied to clipboard

stable clip paths

Open Fil opened this issue 2 years ago • 2 comments

new description!

By using (and reusing) the same clip-path when two renders are clipped with the same shape, we get:

  • smaller files
  • better performance during pointer interactions

Tested manually against #1623. The only reason why I based this PR on #1623 originally is that this forms a good test case: demo); for the same reason, merging this will change the snapshots for the new tests added in #1623, but they are independent concerns.

Fil avatar May 24 '23 06:05 Fil

Marking as a draft since it needs to be rebased on #1623.

Fil avatar Sep 13 '23 22:09 Fil

This seems ready now.

Fil avatar Sep 14 '23 09:09 Fil

rebased

Fil avatar Aug 01 '24 16:08 Fil

This was clearly over engineered. I don't think we even need a WeakMap, as we could just do a quick check through the DOM with querySelector; we just need to pass the type of clip (frame, sphere) in a class or in the id, then select against it e.g. with [id^=plot-clip-sphere-].

Fil avatar Aug 05 '24 01:08 Fil

I don't think we even need a WeakMap, as we could just do a quick check through the DOM with querySelector

Why do you think that (going through the DOM) is simpler or faster than a WeakMap?

mbostock avatar Aug 05 '24 01:08 mbostock

Using the DOM feels simpler [information-wise], in the sense that we wouldn't need to track the association of type of clip and clipPath in parallel, as we could just look it up. (It's hard to remember what I had in mind a year ago, though — I think I was probably trying to change the output as little as possible.) But I'm happy with this version too.

Fil avatar Aug 05 '24 01:08 Fil

Using the DOM feels simpler [information-wise], in the sense that we wouldn't need to track the association of type of clip and clipPath in parallel, as we could just look it up.

I don’t follow that the DOM is simpler. In the DOM approach, you have to use some (serializable) identifier in the DOM such as an id prefix ([id^=plot-clip-sphere-]), and then use querySelector to find it. In the WeakMap approach, you use a symbolic identifier (the reference to the WeakMap) and you use map.get to find it. The main difference I see is that the WeakMap approach is completely symbolic and hidden, whereas the DOM approach exposes its implementation, and requires pattern matching or other messiness via querySelector since it’s not a symbolic reference.

mbostock avatar Aug 05 '24 01:08 mbostock

The way I think about the WeakMap is that there’s a context.frameClip and context.projectionClip property that are lazily set in getFrameClip and getProjectionClip. The only thing the WeakMap provides is that these properties aren’t publicly exposed.

mbostock avatar Aug 05 '24 01:08 mbostock

Makes sense; you're right that there is as much tracking in both cases, just differently. To be clear my comment about over engineering was about the original introduction of the parallel tracking with Map; I could have used a className instead, but indeed it would have been visible in the output.

Fil avatar Aug 05 '24 01:08 Fil