zepumph
commented
1 year ago This is a great idea! I love it. What I was thinking was a couple of items:
- There are 2 usages of
target.in the code, and they both relate to the same event (hitting the ground). They could be closer like this:
Index: js/common/model/Trajectory.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Trajectory.js b/js/common/model/Trajectory.js
--- a/js/common/model/Trajectory.js (revision 34dc139c76ea020ff77aa744d49e363a9b31fd70)
+++ b/js/common/model/Trajectory.js (date 1669845904392)
@@ -222,8 +222,6 @@
assert && assert( this.projectileObject, 'there must be a projectile object' );
assert && assert( !landed, 'a projectile should only land once!' );
- this.hasHitTarget = this.target.isWithinTarget( this.horizontalDisplacement );
-
// TODO: Inline this listener into the right spot (when projectile has landed), https://github.com/phetsims/projectile-motion/issues/291
this.trajectoryLandedEmitter.emit( this );
landed = true;
@@ -453,12 +451,12 @@
this.projectileObject.dataPointProperty.set( currentDataPoint );
}
- // if it has just reached the end, check if landed on target
+ // if it has just reached the ground, check if landed on target
else if ( !this.projectileObject.checkedScore ) {
this.numberOfMovingProjectilesProperty.value--;
- this.target.scoreIfWithinTarget(
- this.projectileObject.dataPointProperty.get().position.x
- );
+ const displacement = this.projectileObject.dataPointProperty.get().position.x;
+ this.target.scoreIfWithinTarget( displacement );
+ this.hasHitTarget = this.target.isWithinTarget( displacement );
this.projectileObject.checkedScore = true;
}
}
- From that patch why not try to factor out a function that both of those can work with?
- Once we do that it could be nice to pass Trajectory that single bound function instead of a whole Target.
Let me know if you have any questions. We can work async or sync depending on what you're thinking.
matthew-blackman
Currently the model for the target is being passed into the constructor of each Trajectory. @zepumph and I believe this to be overkill, since the only thing Trajectory needs is to call a public function on Target. We are planning to refactor this behavior so that each Trajectory does not have the full Target model being passed into it.