Allow Path/Image to take TReadOnlyProperty with shapeProperty/imageProperty

[jonathanolson]

It looks like these will need to take a slightly more unhooked approach, due to:

1. Imageable (where _image lives, and _imageProperty will) is mixed by non-Node things. TinyForwardingProperty.setTargetProperty has a lot of assumptions about Node/NodeLike. SpriteImage is not Node-like. @zepumph I'm curious about review, I wasn't expecting the setTargetProperty implementation (... it looks safe to me to pass null in?)
2. image.image is... not what you pass in (in many cases). It coerces things to HTMLImageElement/HTMLCanvasElement (and mipmap things get stored elsewhere). Long story short, we need to still store _image, while _imageProperty can be this pure and amazing representation of "what is the current input" (which is great and valuable).
3. Path also coerces (away from string)
4. Path subtypes lazily construct actual Shape instances (when asked, and track whether it needs to be regenerated). This requires extra storage.

Finishing testing and adding some implementations in sims (where we were clearly linking something that could be a Property/DerivedProperty).

[jonathanolson]

Additionally in phetsims/beers-law-lab@7c794ec, I had to add strictAxonDependencies:false to something (otherwise... reading Node bounds inside of a DerivedProperty would be prohibited?).

@samreid thoughts on whether this issue (related to phetsims/axon#441) qualifies as a good catch (hey, should we listen to the bodyNode/probeNode bounds?) or as noise?

[jonathanolson]

@zepumph, available to review?

[samreid]

@samreid thoughts on whether this issue (related to phetsims/axon#441) qualifies as a good catch (hey, should we listen to the bodyNode/probeNode bounds?) or as noise?

It seems unfortunate that the framework cannot figure out that listening to body.positionProperty counts towards listening for the bodyNode.centerX, but on the other hand, it is possible to imagine those getting out of sync. For instance, if the listener order was different, then you could get the callback on body.positionProperty having changed before the bodyNode.centerX has updated, then it would be buggy.

[zepumph]

From review:

• I'd like to talk about how Imageable supports (or doesn't support) null. It doesn't seem like it has a place there, but nulling out a shape in Path seemed useful. I'm curious about the discrepancy. For example asserting we have an image set:

  scenery/js/nodes/Imageable.ts

  Line 242 in b356bc1

  assert && assert( this._image !== null );

  .
• On slack you mentioned that it was awkward to have ParsedImage as the type for getHitTestData. I'll leave it up to you if you want to expand the definition of what a ParsedImage is, or hard code them for that function. Either sounds great to me.

Thus far I have only completed the Path and Imageable files review.

[zepumph]

• Here are some spots I found that may look nicer using this newer pattern, are you familiar enough with any of them to refactor them? I think having a couple more usages before closing sounds pretty nice:

  • https://github.com/phetsims/center-and-variability/blob/efb391ba28295aeba0f52f72119f34bdcf02b86f/js/common/view/CAVNumberLineNode.ts#L52-L66
  • https://github.com/phetsims/circuit-construction-kit-common/blob/eb8a4916be8e6d169e813a98a96c465032d6515f/js/view/CCKCLightBulbNode.ts#L181-L198
  • https://github.com/phetsims/circuit-construction-kit-common/blob/11ef57ab8e430fad8b5ffbd22921be3045a0c867/js/view/FuseNode.ts#L163-L167
  • https://github.com/phetsims/curve-fitting/blob/11782cde78d25957f63a4f7e1644ccb39e8d7fb6/js/curve-fitting/view/CurveNode.js#L35-L50
  • https://github.com/phetsims/fluid-pressure-and-flow/blob/26cbe7264c054f51854867b721bbef0cacefa193/js/under-pressure/view/TrapezoidPoolWaterNode.js#L32-L54
  • https://github.com/phetsims/forces-and-motion-basics/blob/74cf55d82c077387eb98a7a7caf95425f3575f98/js/motion/view/WaterBucketNode.js#L91-L102
  • https://github.com/phetsims/geometric-optics/blob/f4abc13a6bd32587a49ac6557f9da4c812482d86/js/lens/view/LensNode.ts#L76-L80
  • https://github.com/phetsims/geometric-optics/blob/f4abc13a6bd32587a49ac6557f9da4c812482d86/js/mirror/view/MirrorNode.ts#L73-L76
  • https://github.com/phetsims/joist/blob/b7425bbddee0006ead535c8908535f5d623de7df/js/Helper.ts#L553-L560

And I stopped looking there (made it to joist/)

---

• Here are some usages for 'shape' that maybe want to include 'shapeProperty' too.

  • https://github.com/phetsims/density-buoyancy-common/blob/bd98c9901305ddece235f29d74adcbb3e73222af/js/buoyancy/model/Boat.ts#L29-L28
  • https://github.com/phetsims/density-buoyancy-common/blob/bd98c9901305ddece235f29d74adcbb3e73222af/js/buoyancy/model/Bottle.ts#L172
  • https://github.com/phetsims/density-buoyancy-common/blob/44aceaa70b22a6eba736e2bf2f3797b4c764854d/js/common/model/Cone.ts#L29
  • All other shapes in buoyancy

  • scenery/js/nodes/Circle.ts

    Line 334 in 6b42241

    Circle.prototype.drawableMarkFlags = Path.prototype.drawableMarkFlags.concat( [ 'radius' ] ).filter( flag => flag !== 'shape' );

  • scenery/js/nodes/Line.ts

    Line 528 in 6b42241

    Line.prototype.drawableMarkFlags = Path.prototype.drawableMarkFlags.concat( [ 'line', 'p1', 'p2', 'x1', 'x2', 'y1', 'y2' ] ).filter( flag => flag !== 'shape' );

  • scenery/js/nodes/Path.ts

    Line 608 in d5a8b54

    Path.prototype.drawableMarkFlags = [ ...Node.prototype.drawableMarkFlags, ...PAINTABLE_DRAWABLE_MARK_FLAGS, 'shape' ];

  • scenery/js/nodes/Rectangle.ts

    Line 1031 in 16003c1

    Rectangle.prototype.drawableMarkFlags = Path.prototype.drawableMarkFlags.concat( [ 'x', 'y', 'width', 'height', 'cornerXRadius', 'cornerYRadius' ] ).filter( flag => flag !== 'shape' );

• Would it be easy to add support for this for highlights too while we are add it? Probably not and worth a separate issue but I thought I'd ask:

  scenery/js/overlays/HighlightOverlay.ts

  Line 29 in 601793e

  type HighlightMode = null | 'bounds' | 'node' | 'shape' | 'invisible';

Otherwise I completed the review and everything looks really nice. Thank you!

[jonathanolson]

I'd like to talk about how Imageable supports (or doesn't support) null. It doesn't seem like it has a place there, but nulling out a shape in Path seemed useful. I'm curious about the discrepancy. For example asserting we have an image set:

A lot of code is needed to support a null value there. It would take more plumbing to adjust (SVG path with empty path info is fine, SVG image with empty src seems to have issues). I realize there is a discrepancy, but I would resolve that discrepancy by not letting Path take null (and instead use an empty Shape).

[jonathanolson]

Here are some usages for 'shape' that maybe want to include 'shapeProperty' too.

The cases of drawableMarkFlags don't actually include 'shapeProperty', so there is no need to remove them.

[jonathanolson]

Here are some usages for 'shape' that maybe want to include 'shapeProperty' too.

The Density/Buoyancy ones aren't based on Path, so that won't work either:

[jonathanolson]

Created an issue for potential chip-away to refactor using the new patterns as noted above: #1651

[jonathanolson]

Created an issue for the highlight overlay shapeProperty in #1652.

[jonathanolson]

@zepumph thoughts on the above? Is everything handled?

[zepumph]

I think this all looks great. Thanks for handling this.