search

phetsims / dot

A math library with a focus on mutable and immutable linear algebra for 2D and 3D applications.
http://scenerystack.org/
MIT License
13 stars 6 forks source link

Improve LinearFunction API #112

Closed samreid closed 3 years ago

samreid commented 3 years ago

During TypeScript porting with @jbphet, we saw that TypeScript was confused by our usage of LinearFunction. We say it is a @constructor and invoke it with new but we return a different function from the constructor. This causes type errors in TypeScript. We found things get better if we remove @constructor and new. We may also want to return a rich object with "evaluate" and "inverse" methods, or rename it to createLinearFunction.

samreid commented 3 years ago

I'm planning to turn LinearFunction into a class with an evaluate method. It will be instantiated with new. This will match up with other interfaces we use such as Transform1 and PiecewiseLinearFunction. There are around 70+ usages of LinearFunction, so they will need to all be updated.

samreid commented 3 years ago

Everything is passing aqua locally, I think this is ready for commit.

samreid commented 3 years ago

I wasn't sure who to ask for review, so I ran 

_.sample('JG, JB, JO, CK, SR, MK, CM'.split(' '))
'JG,'

@jessegreenberg can you please review? Close if all is well.

samreid commented 3 years ago

On second thought, I noticed the original author of LinearFunction was @pixelzoom, so perhaps @pixelzoom should review. The problem is described in https://github.com/phetsims/dot/issues/112#issue-1033972971 and the proposed solution (which was implemented) is described in https://github.com/phetsims/dot/issues/112#issuecomment-957542380.

pixelzoom commented 3 years ago

The proposed solution sounds great to me. I've always hated the original implementation - which was a consensus as I recall, to make call sites less verbose.

samreid commented 3 years ago

Thanks, closing.

samreid commented 3 years ago

Upon reflection, it is unclear whether @pixelzoom reviewed the implementation or the proposal only. @pixelzoom can you clarify? Please close if all is well.

pixelzoom commented 3 years ago

Implementation looks good, closing.