Remove ShakerParticles.setValueToSim after #103 complete

[pixelzoom]

Here's ShakerParticles.setValueToSim and its documentation, added by @samreid:

    /**
     * For together, load the state of all the particles.
     * This is declared here instead of in together.js because of the methods + logic required here,
     * and since the concentration-api.js file does cannot easily replicate the code below since
     * it runs in a preload script, and types such as ShakerParticle are not available globally.
     * The argument is defined in concentration-api.js.
     * TODO: To fully support save/load, we must also capture the particle velocities, solute type, orientation, etc.
     * @param {Object} value - the state as loaded by concentration-api.js
     */
    setValueToSim: function( value ) {

Several questions/requests:

(1) I don't understand "This is declared here instead of in together.js". together.js is a general API. Ignoring the reasons why it's not declared in together.js, why would something sim-specific like this ever be declared in together.js?

(2) "The argument is defined in concentration-api.js." Can you at least indicate what we're looking for in concentration-api.js? I'm guessing that param value is probably an array of concentration-api.ShakerParticle.

(3) If this is an array, why is the parameter named value? Shouldn't it be plural? value[ i ] looks very odd.

(4) Is there a reason why this function is named setValueToSim? Is that specific name required by some part of together/tandem? If not, how about calling this something that makes more sense, like loadShakerParticles? Or if it needs to be a name that's general and used across sims, how about loadState? Whatever the name, the documentation starts with "load the state", so why not name the function accordingly?

[pixelzoom]

@jbphet please feel free to chime in.

[pixelzoom]

This issue also illustrates the importance of addressing https://github.com/phetsims/together/issues/125 (Should wrapper types be renamed?) Parameter value is presumably poorly documented because @param {ShakerParticle[]} value would be confusing - we wouldn't know which ShakerParticle we're referring to, the one in concentration-api.js or the one in ShakerParticle.js.

[samreid]

Unassigning until we are working on together again.

[pixelzoom]

Since beers-law-lab has now been fully instrumented, presumably it's time to revisit this.

[pixelzoom]

Related to https://github.com/phetsims/beers-law-lab/issues/99

[samreid]

Specific answers below, then a general summary and recommendation afterwards:

(1) I don't understand "This is declared here instead of in together.js". together.js is a general API. Ignoring the reasons why it's not declared in together.js, why would something sim-specific like this ever be declared in together.js?

This was written at a time when I was considering naming the together repo as together.js. The implementation could be in the beers-law-labs-api.js file (like other sim-specific wrapper types), not together.js.

(2) "The argument is defined in concentration-api.js." Can you at least indicate what we're looking for in concentration-api.js? I'm guessing that param value is probably an array of concentration-api.ShakerParticle.

It is the object created by toStateObject in beers-law-lab-api.js

    toStateObject: function( shakerParticles ) {
      var stateObject = [];
      for ( var i = 0; i < shakerParticles.particles.length; i++ ) {
        var particle = shakerParticles.particles[ i ];
        var position = particle.locationProperty.value;
        stateObject.push( { x: position.x, y: position.y } );
      }
      return stateObject;
    }

(3) If this is an array, why is the parameter named value? Shouldn't it be plural? value[ i ] looks very odd.

Yes, it should be named something like serializedShakerParticlesStateObject or particleStates.

(4) Is there a reason why this function is named setValueToSim? Is that specific name required by some part of together/tandem? If not, how about calling this something that makes more sense, like loadShakerParticles? Or if it needs to be a name that's general and used across sims, how about loadState? Whatever the name, the documentation starts with "load the state", so why not name the function accordingly?

It has been renamed to ShakerParticles.setValue.

Summary and recommendation: I wrote this adapter as a workaround since we did not have support for object groups. A better direction to move would be to instrument the individual particles (as has been done for John Travoltage's electrons), if the performance demands are acceptable.

[samreid]

Once we instrument ShakerParticle.js in #103, then the above workaround functions can be removed.

[samreid]

Removed since the work is covered by #103, closing.