phetsims/scenery

scenery fill/stroke doesn't support TinyProperty

Opened this issue · 4 comments

zepumph commented

I was surprised to see many spots with instanceof ReadOnlyProperty without any notion of TinyProperty. I'll take a look. I got bit for ~2 hours on this over in phetsims/buoyancy#96 thinking I had done something wrong in THREE.js when actually it was just that I was using a TinyProperty<Color> instead of a Property<Color>.

zepumph commented

For review:

  • I ended up looking for all usages of instanceof ReadOnlyProperty || instanceof TinyProperty. So it leaked a bit outside of scenery
  • In Paintable, I noticed had to add Paint as a return type for fill and stroke. Do we need the specific types here too? Right now it is like LinearGradient | RadialGradient | Pattern | Paint
  • I'm really happy about how typesafe this makes a lot of our code. Note the couple of type cast as calls I could get rid of.
  • Please note how I created a Type parameter for isTReadOnlyProperty so that we could pass through the non Property type that we wanted to pull out of the return type. It is hard coded, but I think it is better than nothing (needed for Paintable changes with the return type).
  • I did end up finding some pixel comparison changes, but they were very very slight from this change, and so I still decided to commit. Here is an example of what changed in BASE and GE:E

1download
2download
3download
4download

@jonathanolson can you please review the above commits? Thanks.

zepumph commented

This issue mainly started because I thought that b8f7e12 would be a simple and easy change, but it actually opened up can of worms. I'm glad that I hit this instead of someone else since it seems so obvious that TReadOnlyProperty should behave the same when passing into scenery options.

zepumph commented

I'll test snapshot again on firefox, because that may be chromes fault (says @jonathanolson).

zepumph commented

The pixel comparisons on firefox showed no changes. Thanks for the tip @jonathanolson.