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

Bounds2.dirtyFromPool() doesn't work #91

Closed jbphet closed 3 years ago

jbphet commented 5 years ago

@chrisklus and I were working to reduce allocations in Energy Forms and Changes, and we tried to use Bounds2.dirtyFromPool, and it threw an assertion. I suspect that all that is needed is a no-argument constructor, but there may be a reason that this doesn't exist, so I'm logging an issue and assigning to @jonathanolson, since he created both Bounds2 and Poolable.

chrisklus commented 5 years ago

@jbphet and I noticed that Vector2's poolable mixInto() call has default values, but Bounds2 did not. We added those and now Bounds2.dirtyFromPool works.

@jonathanolson please review.

samreid commented 5 years ago

The above commit uses these default values for Bounds2:

[ Number.POSITIVE_INFINITY, Number.POSITIVE_INFINITY, Number.NEGATIVE_INFINITY, Number.NEGATIVE_INFINITY ]

It is unclear why these are desirable/correct defaults. Can you please document/clarify?

chrisklus commented 5 years ago

We were using Bounds2.dirtyFromPool as a replacement for allocating new Bounds as Bounds2.NOTHING, which contains those values.

Do you think [ 0, 0, 0, 0 ] would be more appropriate?

samreid commented 5 years ago

We were using Bounds2.dirtyFromPool as a replacement for allocating new Bounds as Bounds2.NOTHING, which contains those values.

Bounds2.NOTHING seems reasonable, perhaps document it and use its values like:

defaultArguments: [ Bounds2.NOTHING.minX, etc...]
chrisklus commented 5 years ago

Fixed, thanks @samreid.

jonathanolson commented 3 years ago

This looks great to me, thanks!