sloppy TTypes

[pixelzoom]

Noted while working on #212

Similar to phetsims/axon#159, I see TTypes that are reaching into arbitrary levels of the concrete type hierarchy to grab fields.

E.g. TPrecipitateParticle is for PrecipitateParticle, a subtype of SoluteParticle. Serialization of TPrecipitateParticle is reaching into SoluteParticle to grab this.orientation and this.locationProperty. Based on previous discussions, shouldn't SoluteParticle fields be handled by a new TSoluteParticle, which TPrecipitateParticle extends?

The same problem occurs with TShakerParticle, where ShakerParticle is also a subtype of SoluteParticle. Grabbing grab this.orientation and this.locationPropertyis duplicated there. So this seems to be further evidence that there should be a TSoluteParticle.

It also seems like the TType hierarchy should mirror the concrete type hierarchy. Otherwise you break encapsulation, and end up with duplication (like this case).

Please clarify. What are best practices for TType implementation?

EDIT by @samreid: Added whitespace in 1st line to separate onhttp://

[pixelzoom]

Same problem with TPrecipitate. It reaches into the supertype of Precipitate.

[samreid]

Yes, we should introduce TTypes for the supertypes. @pixelzoom would you like to do so, or shall I give it a try and reassign to you for review?

[pixelzoom]

@samreid I don't have time, so yes, please address:

• Fix the problem throughout beers-law-lab
• Review all TTypes for this problem - I suspect it occurs elsewhere
• Update documentation (how to instrument? code review?) to describe best practices for TTypes, including this specific problem

[samreid]

TBeersLawSolution looks good.
TConcentrationModel looks good.
TSolute looks good

• TPrecipitate should extend new class TParticles
• TPrecipitateParticle should extend new class TSoluteParticle
• TShaker should extend new class TMovable
• TShakerParticle should extend new class TSoluteParticle
• TShakerParticles should extend TParticles (created above)

[pixelzoom]

... and each of the above Types should access only fields and methods in its associated type, no reaching into supertypes.

[pixelzoom]

I said:

Review all TTypes for this problem - I suspect it occurs elsewhere

Indeed, this problem is not isolated to beers-law-lab. Examples:

• build-an-atom: TBAAGameChallenge reaches into supertype BAAGameState of BAAGameChallenge, even though TBAAGameState exists.

• charges-and-fields: TChargedParticle reaches into supertype ModelElement of ChargedParticle, even though TModelElement exists.

[samreid]

TPrecipitate calls instance.removeAllParticles which is defined in Precipitate but not in Particles. Likewise TShakerParticles calls instance.removeAllParticles(); which is defined in ShakerParticles but not Particles. JS does not have the notion of abstract methods or types, how should we proceed?

[pixelzoom]

@samreid asked in #213 (comment):

... how should we proceed?

I don't understand what problem you're referring to. This issue deals with serializing fields that aren't defined in the associated type. Referring to inherited fields or methods (which I believe is what you're referring to) in TType function calls seems fine.

[zepumph]

Could this be put on hold until we redesign BLL?

[pixelzoom]

@zepumph asked:

Could this be put on hold until we redesign BLL?

It could. But I think we're close to closing this issue, dependent on @samreid's answer to #213 (comment).

[samreid]

After review, it wasn't clear that all of the steps in #213 (comment) were warranted. Instead, I started with the specific recommendations in #213 (comment) in the preceding commits.

@pixelzoom can you please review these commits?

[samreid]

I introduced TParticles in the preceding commits. I also added Particles.addParticle because it seemed odd to have TParticles calling a method which did not exist on Particles. @pixelzoom can you please take a look?

[samreid]

I also added a new section to the "how to instrument docs" https://github.com/phetsims/phet-io/blob/master/doc/how-to-instrument-a-phet-simulation-for-phet-io.md#ttypes-should-use-proper-inheritance @pixelzoom can you please take a look?

[pixelzoom]

@samreid said:

I introduced TParticles in the preceding commits. I also added Particles.addParticle ...

Two problems with Particles/TParticles, both fixed in the above commit:

(1) TParticles is calling instance.removeAllParticles, which is not defined in Particles, but in its subtypes. I added abstract method removeAllParticles to Particles, so that TParticles is not using anything that isn't defined in Particles.

(2) Particles addParticle was added above. It already existed in Shaker (subtype of Particles), so Shaker is overriding addParticles. The implementations in the Particles and Shaker were identical, so I removed addParticle from Shaker.

@samreid please review, close if all looks OK.

[pixelzoom]

@samreid said:

I also added a new section to the "how to instrument docs" ... @pixelzoom can you please take a look?

Looks good, thanks.

[samreid]

Here is Precipitate.removeAllParticles:

    /**
     * Removes all particles from the precipitate.
     * @public
     * @override
     */
    removeAllParticles: function() {
      if ( this.particles.length > 0 ) {
        this.removeParticles( this.particles.length );
      }
    },

...
    /**
     * Removes particles from the precipitate.
     * @param {number} numberToRemove
     * @private
     */
    removeParticles: function( numberToRemove ) {
      assert && assert( numberToRemove > 0 && numberToRemove <= this.particles.length,
        'invalid numberToRemove: ' + numberToRemove );

      var removedParticles = this.particles.splice( this.particles.length - numberToRemove, numberToRemove );
      assert && assert( removedParticles && removedParticles.length === numberToRemove );

      for ( var i = 0; i < removedParticles.length; i++ ) {
        removedParticles[ i ].dispose();
      }
    },

Here is ShakerParticles.removeAllParticles

    /**
     * @public
     * @override
     */
    removeAllParticles: function() {
      var particles = this.particles.slice( 0 );
      for ( var i = 0; i < particles.length; i++ ) {
        this.removeParticle( particles[ i ] );
      }
      this.fireChanged();
    }

...

    // @private
    removeParticle: function( particle ) {
      this.particles.splice( this.particles.indexOf( particle ), 1 );
      particle.dispose();
    },

Some questions:

1. Why is fireChanged called in ShakerParticles but not Precipitate?
2. Should we move ShakerParticles.removeAllParticles and ShakerParticles.removeParticle up to Particles, then delete around 25 lines of code in Precipitate?

[pixelzoom]

@samreid asked:

(1) Why is fireChanged called in ShakerParticles but not Precipitate?

ShakerParticles calls fireChanged in step (an optimization that results in 1 fire per step, instead of 1 per particle) and in removeAllParticles (because step is short circuited when there are no particles).

Precipitate handles the fireChanged in updateParticles, which adds/removes particles based on the amount of precipitate. This optimization results in 1 fire per call to updateParticles, instead of 1 fire per particle added/removed.

(2) Should we move ShakerParticles.removeAllParticles and ShakerParticles.removeParticle up to Particles, then delete around 25 lines of code in Precipitate?

The implementation of removeAllParticles is very different for ShakerParticles and Precipitate. Precipitate removeAllParticles is optimized to perform one slice. If you made the proposed change, we'd be removing particles individually, requiring one splice for each particle -- unclear if this would have a performance impact.

If we want to move more code into Particles, then we should look at applying the Precipitate removeParticles approach to both Precipitate and ShakerParticles. I'll investigate, but note that this will result in no change to TTypes, and we're fixing what's not broken.

[pixelzoom]

I said:

I'll investigate, but note that this will result in no change to TTypes, and we're fixing what's not broken.

Decided I'm not going to do this. ShakerParticles and Precipitate are different for good reasons.

@samreid There's a fundamental problem in TParticles. The implementation of clearChildInstances and addParticle are appropriate for ShakerParticles, but not for Precipitate. Precipitate updates itself based on model Properties -- it has no step method, and its removeAllChildren method is intended to be private. Let's discuss.

EDIT: Looks like TPrecipitate had this same problem prior to introducing TParticles. That is, its clearChildInstances and addChildInstance consisted entirely of calls to methods that it should not be calling.

[pixelzoom]

@samreid and I worked on the TParticles issue (#213 (comment)) via Slack. We ultimately deleted TParticles and TPrecipitate, and moved all functionality into TShakerParticles.

We decided not to save state of individual Precipitate particles, because Precipitate particles are derived from the model. Problems: (1) depending on whether we restored model or view first, we could end up with different behavior; (2) saving/restoring individual particles would require using methods that are private to Precipitate.

Nothing else to do here, closing.