phetsims/scenery-phet

add tailDash feature to ArrowNode

Closed this issue · 6 comments

Motivated by decisions in phetsims/vector-addition#177 (comment).

  • tailDash value will be similar to SCENERY/Line lineDash, e.g. tailDash: [ 3, 2 ]
  • ArrowNode will continue to be a subclass of Path, described by a single Shape
  • dash will be constructed from tail to tip, so that any partial dash (or space) is near the tip

Ugh, this is getting really complicated. ArrowNode and ArrowShape are full of optimizations that assume that the Shape can be described as a reusable set of Vector2 instances. And that's not possible if the tail is dashed, it assumes that the tail is solid. @ariel-phet FYI.

Slack discussion:

Chris Malley 1:47 PM
I’ve been asked to add a tailDash option to ArrowNode, see #533. I’ve discovered that the current implementation is very tightly coupled to describing the Shape as a reusable {Vector2[]}, which assumes a solid tail. The documentation indicates that this is for performance reasons. Does anyone recall which sims rely on this?

Jonathan Olson 1:48 PM
I think it's a general-performance thing (a lot of sims update arrows every frame), I don't recall the original sim that triggered those changes, but I know it's not specific to it

Chris Malley 1:49 PM
I don’t see how this optimization could be possible with a dashed tail. So it seems like my options are (1) add a totally separate code-path inside ArrowNode (and ArrowShape?) if options.tailDash is provided, or (2) roll my own VectorNode that is specific to vector-addition and supports solid and dashed tails.

Jonathan Olson 1:52 PM
I could conceive of other simulations wanting a dashed tail

Chris Malley 1:52 PM
Me too. That was the thinking in adding to ArrowNode. But it’s turning into a major project.
And if sims are really relying on this optimization, will it be OK if there’s no optimization for dashed tails? The optimization is intended to reduce the number of Vector2 instances created, reusing instances. ArrowShape (and ArrowNode) currently assume that they can iterate over the {Vector2[]} as a set of Shape.lineTo calls. For dashed tails, I was going down the path of using Shape.rect for the dash segments.

Jonathan Olson 1:57 PM
I agree it seems like a major project, but it also seems like one that would be useful for the future. I don't think there's any way around the optimization bit for dashed tails.

Chris Malley 1:58 PM
Please clarify - by “any way around” do you mean “no way to implement” or “we must support optimization” ?

Jonathan Olson 2:00 PM
I mean "it's untenable to use the same Vector2[] approach for dashed tails, so performance for dashed tails will always be worse"

The conclusion of the above Slack conversation was to try creating DashedArrowShape, and use it instead of ArrowShape in ArrowNode, if option tailDash.

I spent ~3 hours on this and got nowhere. Among other things, I tried creating DashedArrowShape. It was a horrible mess, and I bailed.

This feature is not currently feasible to add to ArrowNode/ArrowShape or LineArrowNode. If this is a feature that is generally needed by sims, then I recommend a complete redesign and reimplementation of ArrowNode - a major project. That's not something that I currently have time/interest for. So to address the needs of Vector Addition in phetsims/vector-addition#177, we'll need to incur some technical debt and do something sim-specific to move forward.

@ariel-phet back to you to decide what to do with this issue (close, defer,...)

@pixelzoom I actually think a sim specific implementation is totally appropriate currently. We can wait until some recalcitrant designer (ie me) demands dashed arrows in another sim before making a general solution. I think what you will have done in Vector Addition will be informative to that general solution when it becomes desired.

This might also potentially be nice work for someone like @brandonLi8 in the future, to take on something that is heavy common code lifting with some architectural direction.

Marking deferred currently.

The original idea proved to be both too complicated and unnecessary. And it's not clear what (if anything) should be done. So closing this issue. If a similar feature is needed in the future, we can revisit.