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

Launching sim during kicks leads to wrong person #128

Closed
Nancy-Salpepi opened this issue Apr 8, 2022 · 4 comments
Closed

Launching sim during kicks leads to wrong person #128

Nancy-Salpepi opened this issue Apr 8, 2022 · 4 comments
Labels
dev:help-wanted Extra attention is needed dev:phet-io type:bug Something isn't working

Comments

@Nancy-Salpepi
Copy link

Nancy-Salpepi commented Apr 8, 2022

Test device
MacBook Air (m1 chip)

Operating System
12.3.1

Browser
safari

Problem description
For phetsims/qa#795
In studio, if you launch the sim during a kick, the wrong person appears. After kicking all the balls in the launched sim, the last kicker will be present on the screen. The number of balls and the number of cards will be correct.

This doesn't occur with the Kick 5 button.

Steps to reproduce

  1. In studio, on either screen, press the Kick 1 button.
  2. Launch the sim before the ball lands.

Visuals

wrongperson.mp4

Screen Shot 2022-04-08 at 2 33 26 PM

@Nancy-Salpepi Nancy-Salpepi added the type:bug Something isn't working label Apr 8, 2022
@amanda-phet amanda-phet added this to the 1.1 Publication milestone Apr 11, 2022
@Nancy-Salpepi
Copy link
Author

Also seen in the state wrapper

@samreid samreid removed this from the Publish 1.1 milestone Apr 21, 2023
@samreid samreid self-assigned this Apr 21, 2023
@samreid
Copy link
Member

samreid commented Apr 22, 2023

This patch made it a bit further, but still in the downstream sim, advanceLine is not being called when the ball lands.

Subject: [PATCH] Update documentation, see https://github.com/phetsims/center-and-variability/issues/128
---
Index: js/common/model/SoccerPlayer.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/SoccerPlayer.ts b/js/common/model/SoccerPlayer.ts
--- a/js/common/model/SoccerPlayer.ts	(revision 84778c9c89f4c53854174dd4871e95d5fd5b7548)
+++ b/js/common/model/SoccerPlayer.ts	(date 1682172365374)
@@ -16,6 +16,8 @@
 import Property from '../../../../axon/js/Property.js';
 import Pose from './Pose.js';
 import PickRequired from '../../../../phet-core/js/types/PickRequired.js';
+import EnumerationProperty from '../../../../axon/js/EnumerationProperty.js';
+import NullableIO from '../../../../tandem/js/types/NullableIO.js';
 
 type SelfOptions = EmptySelfOptions;
 type SoccerPlayerOptions =
@@ -24,8 +26,10 @@
   & PickRequired<PhetioObjectOptions, 'tandem'>;
 
 export default class SoccerPlayer extends PhetioObject {
-  public readonly poseProperty = new Property<Pose>( Pose.STANDING );
+
+  public readonly poseProperty: EnumerationProperty<Pose>;
   public readonly placeInLineProperty: NumberProperty;
+
   public static readonly SoccerPlayerIO = new IOType( 'SoccerPlayerIO', {
     valueType: SoccerPlayer,
     toStateObject: ( soccerPlayer: SoccerPlayer ) => ( { initialPlaceInLine: soccerPlayer.initialPlaceInLine } ),
@@ -40,7 +44,7 @@
   // Also used to determine the artwork for rendering the SoccerPlayerNode
   public readonly initialPlaceInLine: number;
 
-  public timestampWhenPoisedBegan: number | null = null;
+  public readonly timestampWhenPoisedBeganProperty: Property<number | null>;
 
   public constructor( placeInLine: number, providedOptions: SoccerPlayerOptions ) {
 
@@ -51,10 +55,21 @@
 
     super( options );
 
+    this.poseProperty = new EnumerationProperty( Pose.STANDING, {
+      tandem: options.tandem.createTandem( 'poseProperty' )
+    } );
+
     this.placeInLineProperty = new NumberProperty( placeInLine, {
       tandem: options.tandem.createTandem( 'placeInLineProperty' )
     } );
 
+    this.timestampWhenPoisedBeganProperty = new Property<number | null>( null, {
+      tandem: options.tandem.createTandem( 'timestampWhenPoisedBeganProperty' ),
+      phetioReadOnly: true,
+      phetioDocumentation: 'The timestamp when the player began to be poised to kick the ball.  Null if not poised.',
+      phetioValueType: NullableIO( NumberIO )
+    } );
+
     this.initialPlaceInLine = placeInLine;
   }
 
@@ -64,6 +79,8 @@
 
   public override dispose(): void {
     this.placeInLineProperty.dispose();
+    this.poseProperty.dispose();
+    this.timestampWhenPoisedBeganProperty.dispose();
     super.dispose();
   }
 }
Index: js/common/model/SoccerModel.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/SoccerModel.ts b/js/common/model/SoccerModel.ts
--- a/js/common/model/SoccerModel.ts	(revision 84778c9c89f4c53854174dd4871e95d5fd5b7548)
+++ b/js/common/model/SoccerModel.ts	(date 1682172541208)
@@ -263,14 +263,14 @@
 
         if ( frontPlayer.poseProperty.value === Pose.STANDING ) {
           frontPlayer.poseProperty.value = Pose.POISED_TO_KICK;
-          frontPlayer.timestampWhenPoisedBegan = this.timeProperty.value;
+          frontPlayer.timestampWhenPoisedBeganProperty.value = this.timeProperty.value;
         }
       }
 
       // How long has the front player been poised?
       if ( frontPlayer.poseProperty.value === Pose.POISED_TO_KICK ) {
-        assert && assert( typeof frontPlayer.timestampWhenPoisedBegan === 'number', 'timestampWhenPoisedBegan should be a number' );
-        const elapsedTime = this.timeProperty.value - frontPlayer.timestampWhenPoisedBegan!;
+        assert && assert( typeof frontPlayer.timestampWhenPoisedBeganProperty.value === 'number', 'timestampWhenPoisedBegan should be a number' );
+        const elapsedTime = this.timeProperty.value - frontPlayer.timestampWhenPoisedBeganProperty.value!;
         if ( elapsedTime > 0.075 ) {
 
           const casObject = this.nextBallToKickProperty.value!; // TODO: Probably? See https://github.com/phetsims/center-and-variability/issues/59
@@ -283,6 +283,8 @@
 
   // When a ball lands, or when the next player is supposed to kick (before the ball lands), move the line forward
   private advanceLine(): void {
+    console.log( 'advanceLine');
+    debugger;
 
     // if the previous ball was still in the air, we need to move the line forward so the next player can kick
     const kickingPlayers = this.soccerPlayerGroup.filter( soccerPlayer => soccerPlayer.poseProperty.value === Pose.KICKING );

@samreid samreid removed their assignment Apr 22, 2023
@samreid samreid added the dev:help-wanted Extra attention is needed label Apr 22, 2023
@samreid
Copy link
Member

samreid commented Apr 22, 2023

This is probably caused by this TODO in the code:

this.objectValueBecameNonNullEmitter.addListener( casObject => {
// If the soccer player that kicked that ball was still in line when the ball lands, they can leave the line now.
if ( this.soccerPlayerGroup.includes( this.ballPlayerMap.get( casObject )! ) ) {
this.advanceLine();
}

samreid added a commit that referenced this issue Apr 22, 2023
@samreid
Copy link
Member

samreid commented Apr 27, 2023

This was fixed when we stopped using PhetioGroup for the soccer players, see #160, closing.

@samreid samreid closed this as completed Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev:help-wanted Extra attention is needed dev:phet-io type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants