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

Fix the derived properties for particle counts to use ObservableArray.length #25

Closed jbphet closed 1 year ago

jbphet commented 4 years ago

In the ParticleAtom class there are derived properties that were created in support of phet-io that are basically just the length values of the ObservableArray for that particle type. These should be modified, if possible, to simply use the 'length' property.

See https://github.com/phetsims/axon/issues/149 for more about length property and phet-io.

This issue was logged as part of the effort to make sure all TODO items have associated issues, see https://github.com/phetsims/tasks/issues/1017.

jbphet commented 4 years ago

According to the GitHub history, the TODO in ParticleAtom was originally added by @zepumph, so I'm going to assign this to him to address, prioritize, reassign, or defer as he sees fit.

zepumph commented 4 years ago

At immediate glance, it looks like these DerivedProperties were added just because we didn't yet have OberservableArray's lengthProperty instrumented, my guess is that we can delete these intermediary instrumented Properties.

jbphet commented 1 year ago

Okay, the derived properties for the particle counts have now been removed. I considered removing the Properties such as protonCountProperty from the API and requiring clients of this class to instead use, for example, protons.length, but this would have touched a lot of files and the API seems nicer to me with these Properties in place, so I left them there, and they are now direct read-only references to the lengthProperty values from the observable arrays that contain the particles.

I regression tested Build an Atom and Build a Nucleus, and I tested setting state in the Build an Atom PhET-iO state wrapper, and everything seemed fine. @zepumph - I don't know if more regression testing of the PhET-iO behavior is warranted. Is it? If not, I think this can be closed.

zepumph commented 1 year ago

All is good thanks!