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

MassIO looks overinstrumented and probably has unused parts #300

Closed
samreid opened this issue Jul 31, 2024 · 3 comments
Closed

MassIO looks overinstrumented and probably has unused parts #300

samreid opened this issue Jul 31, 2024 · 3 comments

Comments

@samreid
Copy link
Member

samreid commented Jul 31, 2024

From #123

MassIO is currently:

  public static readonly MassIO = new IOType<Mass, MassIOStateObject>( 'MassIO', {
    valueType: Mass,
    documentation: 'Represents a mass that interacts in the scene, and can potentially float or displace fluid.',
    stateSchema: {
      matrix: Matrix3.Matrix3IO,
      stepMatrix: Matrix3.Matrix3IO,
      originalMatrix: Matrix3.Matrix3IO,
      canMove: BooleanIO,
      tag: MassTag.MassTagIO,
      massShape: EnumerationIO( MassShape ),

      // engine.bodyToStateObject
      position: Vector2.Vector2IO,
      velocity: Vector2.Vector2IO,
      force: Vector2.Vector2IO
    },
    toStateObject( mass: Mass ): MassIOStateObject {
      return combineOptions<MassIOStateObject>( {
        matrix: Matrix3.toStateObject( mass.matrix ),
        stepMatrix: Matrix3.toStateObject( mass.stepMatrix ),
        originalMatrix: Matrix3.toStateObject( mass.originalMatrix ),
        canMove: mass.canMove,
        tag: MassTag.MassTagIO.toStateObject( mass.tag ),
        massShape: EnumerationIO( MassShape ).toStateObject( mass.massShape )
      }, mass.engine.bodyToStateObject( mass.body ) );
    },
    applyState( mass: Mass, obj: MassIOStateObject ) {
      mass.matrix.set( Matrix3.fromStateObject( obj.matrix ) );
      mass.stepMatrix.set( Matrix3.fromStateObject( obj.stepMatrix ) );
      mass.originalMatrix.set( Matrix3.fromStateObject( obj.originalMatrix ) );
      mass.canMove = obj.canMove;
      MassTag.MassTagIO.applyState( mass.tag, obj.tag );
      mass.engine.bodyApplyState( mass.body, obj );
      mass.transformedEmitter.emit();
    },
    stateObjectToCreateElementArguments: ( stateObject: MassIOStateObject ) => [ EnumerationIO( MassShape ).fromStateObject( stateObject.massShape ) ]
  } );

It has 4-5 usages through the project.

  • I suspect stateObjectToCreateElementArguments is unused, since this is not part of a dynamic element pattern.
  • Many attributes like canMove: BooleanIO, and the tag are immutable and therefore not important to capture as part of the state unless for descriptive purposes.
  • It probably just needs a sparse toStateObject and applyState, right?
  • Can it be deleted? Or is this how we capture position state for the masses?
  • If we just need 2-3 elements from it, should they be converted to Property?
@samreid
Copy link
Member Author

samreid commented Aug 6, 2024

Removed canMove/massShape and tag, closing.

@samreid samreid closed this as completed Aug 6, 2024
@phet-dev phet-dev reopened this Aug 7, 2024
@phet-dev
Copy link
Contributor

phet-dev commented Aug 7, 2024

Reopening because there is a TODO marked for this issue.

samreid added a commit that referenced this issue Aug 7, 2024
@samreid
Copy link
Member Author

samreid commented Aug 7, 2024

Fixed, closing.

@samreid samreid closed this as completed Aug 7, 2024
samreid added a commit that referenced this issue Aug 7, 2024
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

2 participants