Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PhET-iO instrumentation of dynamic elements #200

Open
pixelzoom opened this issue Oct 28, 2022 · 4 comments
Open

PhET-iO instrumentation of dynamic elements #200

pixelzoom opened this issue Oct 28, 2022 · 4 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Oct 28, 2022

Related to phetsims/tandem#192 (#192). @amanda-phet @catherinecarter FYI.

Here's a summary of my current understanding of this sim's dynamic elements.

IOTypes that will be needed:

  • TermIO and its subclasses (ConstantTermIO, ObjectTermIO, VariableTermIO)
  • SnapshotIO and its components (PlateSnapshotIO, TermSnapshotIO, TermIO)

PhetioGroups that will be needed, each wrapped in a Collection (the pattern I used in natural-selection):

  • TermGroup and its subclasses (ConstantTermGroup, ObjectTermGroup, VariableTermGroup)
  • TermNodeGroup and its subclasses (ConstantTermNodeGroup, ObjectTermNodeGroup, VariableTermNodeGroup)
  • SnapshotGroup
  • UniversalOperationGroup (or convert to static instances and mutate?)
  • UniversalOperationNodeGroup (or convert to static instances and mutate?)

If animation needs to be stateful for phetsims/tandem#197, these related things need to be examined:

  • SumToZeroNode and it’s twixt.Animation
  • TranslateThenFade (chained twixt.Animations) in UniversalOperationControl
  • faceOpacity animation in SolveItLevelNode
  • RewardNode animation in SolveItLevelNode
  • twixt.TransitionNode is SolveItScreenView
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Oct 28, 2022

There are no plans to work on this until the initial instrumentation has been reviewed, and the initial PhET-iO design has been completed. Reminder that I have not commited to completing dynamic instrumentation.

This issue is also blocked by #204 (PhetioGroup does not support Prototype pattern).

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 7, 2022

There are currently 31 TODOs in the code that reference this issue:

//TODO https://github.com/phetsims/equality-explorer/issues/200

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 7, 2022

@amanda-phet @catherinecarter Before reviewing the Studio tree, read through model.md and implementation-notes.md. The essential elements to understand are:

  • Scene
  • Term
  • TermCreator
  • Snapshot
  • UniversalOperator

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Feb 3, 2023

@kathy-phet @amanda-phet FYI.

Before commiting to finish instrumentation of the equality-explorer suite, I'd like to investigate a new approach to managing dynamic elements. I ran this past @samreid, and he gave it his blessing. A sprint of 3-4 days would allow me to do a proper investigation. The goal of this approach is to minimize the code changes required to manage dynamic elements via PhetioGroup, a major pain point for retrofitting existing sims.

Here's a sketch of the approach...

For each class that requires dynamic elements, create a class that manages a "group" of those instances. Internally, a group will use a PhetioGroup to handle object creation (via multiple patterns, like new and copy, see #204) and disposal. (Note that we're using composition, not inheritance, so that we can hide the API of PhetioGroup.)

Here's an example for the ContantTerm model class in equality-explorer. (These are the elements that can be dragged around that have a constant numeric value, like "1".)

/**
 * ConstantTermGroup managers creation and disposal of ConstantTerm instances.
 */
class ConstantTermGroup {

  // PhetioGroup is an internal detail, not visible outside the group.
  private readonly phetioGroup: PhetioGroup;

  public constructor( ... ) {
    
    // The function that PhetioGroup uses to instantiate a ConstantTerm.
    const createElement = ( tandem: Tandem, ... ) => new ConstantTerm( ... );

    this.phetioGroup = new PhetioGroup( createElement, ... );
  }

  // Replace new ContantTerm( ... ) with constantTermGroup.newElement( ... )
  public newElement( ... ): ConstantTerm {
     return this.phetioGroup.createNextElement( ... );
  }

  // Replace term.copy( ... ) with constantTermGroup.copyElement( ... )
  public copyElement( term: ConstantTerm, ... ): ConstantTerm {
     return this.phetioGroup.createNextElement( ... );
  }

  // Replace term.dispose() with constantTermGroup.disposeElement( term )
  public disposeElement( term: ConstantTerm ) {
    this.phetioGroup.disposeElement( term );
  }

  // etc.
}

The goal would be for newElement to have the same signature as ConstantTerm's constructor, so that replacing new ConstantTerm would look like this, requiring minimal disturbance, and minimal changes to arguments:

-     const term = new ConstantTerm( {
+     const term = constantTermGroup.newConstantTerm( {
        constantValue: Fraction.fromInteger( i )
      } );

Ditto for other methods. Here's an example of how to replace term.copy:

-         const termCopy = term.copy( {
+         const termCopy = constantTermGroup.copy( this.term, {
            diameter: EqualityExplorerConstants.BIG_TERM_DIAMETER
          } );

After creating a group class for each dynamic element class, I would start by using 1 global instance of each group class -- that will be most expedient for testing this approach.

The primary advantage of using globals is that it will minimize changes to code; we don't need to pass around group instances.

The primary disadvantage of using globals is that instances for all screens will be managed by the same group. That could be a problem for sims that (for example) need per-screen notification when an instance is created/disposed. If that proves to be problematic/unacceptable for equality-explorer, then we can create a group per screen, and pass them to where they are needed -- which will involve touching a lot more code. The group class implementation should be the same, regardless of whether we use globals or per-screen instantances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant