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

Nucleus reconfiguration links to mass property instead of nucleon properties #34

Closed Luisav1 closed 2 years ago

Luisav1 commented 2 years ago

In Build a Nucleus, a beta decay changes a proton to a neutron or a neutron to a proton, so overall the mass number stays the same. We don't want the nucleus to reconfigure when the mass number does not change so the multilink below was changed to link only to the massNumberProperty. Visually this does not change the other simulations relying on this since the massNumberProperty is the sum of the protonCountProperty and neutronCountProperty. We wanted to confirm that this is okay across all usages of ParticleAtom and AtomNode.

Original:

// whenever a nucleon is added or removed, update the configuration of the nucleus and the highlight radius
Multilink.multilink( [ atom.protonCountProperty, atom.neutronCountProperty ], () => {
  atom.reconfigureNucleus();
  const radiusOffset = atom.nucleusRadius === 0 ? 0 : 4;
  nucleusFocusHighlight.radius = atom.nucleusRadius + radiusOffset;
} );

New link:

atom.massNumberProperty.link( () => {
  atom.reconfigureNucleus();
  const radiusOffset = atom.nucleusRadius === 0 ? 0 : 4;
  nucleusFocusHighlight.radius = atom.nucleusRadius + radiusOffset;
 } );

You can check this all out in context at line 155 in ElectronShellView.js. @jbphet could you please take a look at this and see if it seems reasonable to change? Let me and @chrisklus know if you want to take a look at it together. Thanks!

jbphet commented 2 years ago

@Luisav1 - Your changes seem reasonable, so I think you should remove the TODO and close this issue. I opened an issue for follow up on this - see #35 - because I think this code probably shouldn't be here at all. I'll work with other devs on that part.

jbphet commented 2 years ago

@Luisav1 - I ended up removing the code in question, please see https://github.com/phetsims/shred/issues/35. The only thing that I'd like to request is that you thoroughly regression test and make sure that the nucleus is correctly reconfiguring in Build a Nucleus in all situations. If that all looks good, you can close this.

Luisav1 commented 2 years ago

Thanks @jbphet! The nucleus ended up not reconfiguring when any nucleons were removed or after any decays occurred in BAN but those were fixed in the commits above. Since the nucleus is correctly reconfiguring in all BAN situations I will close this.