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

Adding options to Particle #36

Closed Luisav1 closed 1 year ago

Luisav1 commented 1 year ago

@marlitas and I added options to set the nucleon radius and the input enabled property.

The nucleon radius option was to decrease the particle's size since BaN has a mini-atom representation of the nucleus. That mini-atom representation should not be interactive, for which we needed an option to set the input enabled property to false by default.

@jbphet Can you quickly look over the changes and close if it all looks good? Thanks!

jbphet commented 1 year ago

This doesn't seem like a great way to do what you're after and still have maintainable code. The Particle class is meant to be a general model of the particles that make up an atom, or an atom as a whole, so adding an option called nucleonRadius doesn't make sense to me, since the particle isn't necessarily a nucleon. There is some code later in the constructor that was altered for this change, and now looks like this:

    this.radiusProperty = new NumberProperty( type === 'electron' || type === 'positron' ? ShredConstants.ELECTRON_RADIUS : options.nucleonRadius, {

So, I think your intent here is to say "if this particle is a nucleon, use this radius value for it". However, the way the code is written, it will use the nucleonRadius for everything that isn't an electron or positron.

I think you probably want to do something more general, like having a initialParticleRadius option that is null by default and, when it is null, the code uses default values based on the type for protons, neutrons, electrons, etc. like it used to do, BUT if a radius value is specified it uses that value. There should also be some clear documentation for the option that specifies this.

Back to you @Luisav1 for next steps.

jbphet commented 1 year ago

Adding @marlitas as an assignee since it's been a few weeks since I responded, and I'm not sure how much @Luisav1 is working on Build a Nucleus (which motivated this change) lately, and @marlitas is on the list of responsible devs.

marlitas commented 1 year ago

Thanks @jbphet, @Luisav1 let's look at this today during our pairing session.

Luisav1 commented 1 year ago

@marlitas and I reverted the commit https://github.com/phetsims/shred/commit/7846bb0a2a0bbb93532965cde5022fb7b9514d5c since we no longer need this. However, this did not address your concerns. Now instead of using option.nucleonRadius it will be using ShredConstants.NUCLEON_RADIUS. If you would would prefer a default of a null radius for any particle that is not an electron or a positron, we are happy to switch the constant into an option that behaves this way. Just to be clear, we saw that any particles that were not electrons have been defaulting to the ShredConstants.NUCLEON_RADIUS since the Property was created in 2014 here https://github.com/phetsims/shred/commit/755bc685f6ecfdc835de82becf79a02b178e7745 (Particle.js line 31). Happy to make this more robust if this does not feel like the right approach anymore.

jbphet commented 1 year ago

The current state of things is acceptable. If we were doing this from scratch today, we'd probably have an option for the radius with a default value for nucleons, but I don't think it's worth the time to migrate to this now. We may well need to do this down the road, but it doesn't seem worth the time now. Closing.