search

phetsims / shred

A library of JavaScript code that is used in PhET simulations that depict atoms, subatomic particles, and atomic structure. This was originally created to contain the code that is shared between the "Build an Atom" and "Isotopes and Atomic Mass" simulations, thought it may be applied to additional simulations in the future.
GNU General Public License v3.0
1 stars 6 forks source link

shred uses non-default layoutBounds #37

Closed pixelzoom closed 11 months ago

pixelzoom commented 1 year ago

And in ShredConstants:

  LAYOUT_BOUNDS: new Bounds2( 0, 0, 768, 464 ),

This is especially concerningbecause these non-default layoutBounds are in a common-code repo (shred), and that will affect all sims that have shred as a dependency. That includes:

I'm not sure how the sims above that use ScreenView.DEFAULT_LAYOUT_BOUNDS are functioning with shred's use of ShredConstants.LAYOUT_BOUNDS.

pixelzoom commented 1 year ago

Responsible dev is listed as @jbphet, so assigning to him to evaluate.

FYI, Aadish G. added ShredConstants.LAYOUT_BOUNDS in https://github.com/phetsims/shred/commit/4d4ba4fd. The commit message indicates that it was moved here from build-an-atom, so @jbphet may have been involved.

pixelzoom commented 1 year ago

What would be ideal is if ShredConstants.LAYOUT_BOUNDS did not exist, and you could tell shred what layoutBounds to use.

marlitas commented 12 months ago

After moving over the Bounds constant in Shred to the usage sites I removed the LAYOUT_BOUNDS constant completely from shred. I'm not quite sure if there was a remaining concern here. It did not look like that constant was being accessed in any other places. I loaded each sim to confirm, but they are loading as I would expect. Over to @pixelzoom to confirm, but I think this issue can be closed.

pixelzoom commented 11 months ago

Great that you were able to get rid of ShredConstants.LAYOUT_BOUNDS. I don't see any obvious problems. Thanks for doing this work. Closing.