Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scenery fill/stroke doesn't support TinyProperty #1612

Open
zepumph opened this issue Feb 16, 2024 · 4 comments
Open

scenery fill/stroke doesn't support TinyProperty #1612

zepumph opened this issue Feb 16, 2024 · 4 comments

Comments

@zepumph
Copy link
Member

zepumph commented Feb 16, 2024

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 zepumph self-assigned this Feb 16, 2024
zepumph added a commit to phetsims/twixt that referenced this issue Feb 17, 2024
zepumph added a commit to phetsims/axon that referenced this issue Feb 17, 2024
zepumph added a commit to phetsims/utterance-queue that referenced this issue Feb 17, 2024
zepumph added a commit to phetsims/joist that referenced this issue Feb 17, 2024
zepumph added a commit that referenced this issue Feb 17, 2024
@zepumph
Copy link
Member Author

zepumph commented Feb 17, 2024

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
Copy link
Member Author

zepumph commented Feb 17, 2024

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 zepumph self-assigned this Feb 19, 2024
@zepumph
Copy link
Member Author

zepumph commented Feb 19, 2024

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

@zepumph
Copy link
Member Author

zepumph commented Feb 22, 2024

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

@zepumph zepumph removed their assignment Feb 22, 2024
zepumph added a commit to phetsims/axon that referenced this issue Feb 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants