While reviewing/grokking the code a did some rather opinionated cleanup/rewrite (more destructuring, more abstractions, less d3). If you prefer to merge without those changes, you can find the previous version without the rewrite here: https://observablehq.com/d/2f312cd0d55bcc31@737
One last note (then I'll finally shut up): The code should also show some minor performance improvements, since it's using native APIs and references the elements directly (instead of calling querySelector for each).
I'm happy to walk you through the changes when you have the time, just DM me on Slack.
Thanks! I'd love to merge less opinionated version (https://observablehq.com/d/2f312cd0d55bcc31@737) but, strangely, I don't see how. There's no merge button.
I'm super happy to see how to incorporate invalidation. It's also interesting to see that the animation can depend on `viewof t` but not respond to changes in `t`. That makes sense, now that I think about it since I guess that `t` and `viewof t` are two different DOM elements. I just wouldn't have that about that, though. I think I've digested most of the code line by line. There's one line that I'm a bit confused about:
onInput.call(scrubber.node());
You're absolutely right about the self-intersection in the NonConvex Mathematica Polyhedron - that's rather the point of that example. Many non-convex polyhedra cannot be unfolded without self-intersection - even in the final net. Also, no one has ever found a convex polyhedron that intersects on unfolding but no one has proven that it can't happen either.
> but, strangely, I don't see how
You can only merge from compare views. This should allow you to merge the changes: https://observablehq.com/compare/29f859a8d9413cc2...2f312cd0d55bcc31@737
> There's one line that I'm a bit confused about: onInput.call(scrubber.node());
The onInput callback expects "this" to refer to the scrubber element. We need to call onInput() once at the start, so that all angles are initialized correctly. This code invokes the callback with the scrubber element as "this" context.
> I'd love to merge less opinionated version
Just so that I can reflect over the feedback: Are there certain parts in the rewrite that put you off, structure wise or style wise? If so, can you go into detail a bit?
> Just so that I can reflect over the feedback: Are there certain parts in the rewrite that put you off, structure wise or style wise? If so, can you go into detail a bit?
No, there's definitely nothing there that put me off. It's just that, as you said yourself, the changes are mostly "opinionated" - one might even say stylistic. When I read code that I've written, I tend to find it very easy to follow, even if it's been some quite some time since I wrote it, because I know and am comfortable with my stylistic conventions. I'd need some pretty compelling reason to change that type of thing; a small change in efficiency is unlikely suffice. Also, there are, to some extent, simple matters of preference. For example, I prefer the animation to reset when the data changes.
Having said that, I think there's a very clear benefit to moving the code that drives the animation into the same cell as that animation so that we can call invalidation. Thus, I've added that to both my unfolding notebooks but I think that's it. Thanks again for that suggestion!
Hmm... I see what you mean an I guess that's what you're doing. If I delete the lines
.select(`#shift_back${e.target}`)
.attr("translation", e.point.toString())
and
.select(`#shift${e.target}`)
.attr("translation", e.point.map((x) => -x).toString());
which perform those updates in my version, though, then it doesn't look right at all. Not sure where that difference is coming from.