phetsims / geometric-optics

Geometric Optics is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
6 stars 5 forks source link

Uninstrument all model `Guides` and `GuideNode` elements #455

Closed veillette closed 1 year ago

veillette commented 1 year ago

This came up during a discussion with @arouinfar. From the 1054 Featured PhET-io Elements, 166 are related to "Guides". If we consider all the 2610 PhET-io Elements, 235 are related to "Guides". It is worth noting that in PhET-brand, guides are not present, even in the preferences dialogue. In PhET brand it is only possible to show the guides by turning on a checkbox related to guides.

I would argue the sheer number of elements related to guide makes the guides less relevant, as it becomes even more difficult to merely attempt to turn the guides as the model elements pollute the namespace.

We propose to unistrument the model elements of the guides.

image

veillette commented 1 year ago

On the view side, it is also not necessary to instrument the visibility property of the guides2Node.

The guides2Node visibility is a derived property that depends on the visibility of the second point.

image

pixelzoom commented 1 year ago

Summary of Zoom discussion with @arouinfar and @veillette:

We also discussed a more general problem, which is the root cause of why do many "guides" tandems appeared in the featured tree. If a parent element has any elements or linked elements that are featured, then that parent element will appear in the featured tree. Including linked elements in this determination is what is causing the problem, and I beleive that linked elements should not be considered when deciding whether a parent element appears in the featured tree. @arouinfar will create a general issue in the studio repo.

pixelzoom commented 1 year ago

The decision about flat mirrors is documented in Optic.ts:

// A flat mirror has infinite focal length and ROC. We approximate a flat mirror as a convex mirror with very-large
// focal length and ROC. Note that these very-large values will be visible in PhET-iO, and it was decided that is OK.
// See https://github.com/phetsims/geometric-optics/issues/227
const FLAT_MIRROR_FINITE_FOCAL_LENGTH = 100000; // positive, to make it convex
const FLAT_MIRROR_FINITE_RADIUS_OF_CURVATURE = 2 * FLAT_MIRROR_FINITE_FOCAL_LENGTH; // ROC = 2 * f

Our decision to expose this in PhET-iO is documented in https://github.com/phetsims/geometric-optics/issues/227#issuecomment-949048738, from 10/21/2021 design meeting:

  • Bogus values (ROC, focal length) in PhET-iO are OK. They can be explained in PhET-iO client guide.

When I did attempt to use physically accurate values, it got complicated, see https://github.com/phetsims/geometric-optics/issues/227#issuecomment-1047445905. @arouinfar and I met, and enumerated the options for proceeding, see https://github.com/phetsims/geometric-optics/issues/227#issuecomment-1048133397. @arouinfar concluded that the complication of a physically-accurate mode was "not worth the hassle", and that we should "hollywood", see https://github.com/phetsims/geometric-optics/issues/227#issuecomment-1054585375.

veillette commented 1 year ago

The above commit uninstrument all guide and guides on the model side.

veillette commented 1 year ago

In summary:

Guides.ts and Guide.ts have been uninstrumented. They do not extend PhetioObject anymore. Their providedOption fields has been removed as their only purposes was to pass a tandem field.
In GOScene.ts, the tandem field was removed from the method initializeGuides as it no longer needs it when instantiated guides.

On the view side, we have uninstrumented GuideNode.ts. Importantly, we have kept the instrumentation of GuidesNode.ts, such that its visibleProperty can be discovered in the Studio tree. The Studio tree has been simplified considerably though image

This table summarizes the number of elements that are no longer present. Header PhET-io Elements related to Guide Featured PhET-io Elements related to Guide
Before 235 166
After 37 16
veillette commented 1 year ago

Assigning to @pixelzoom to review.

pixelzoom commented 1 year ago

Looks great. I made one change in GuideNode.ts, see above commit - param providedOptions is no longer needed for the constructor.

Over to @arouinfar for design review. If this looks OK, you can close this issue.

arouinfar commented 1 year ago

Looks great, thanks @veillette!