phetsims / axon

Axon provides powerful and concise models for interactive simulations, based on observable Properties and related patterns.
MIT License
11 stars 8 forks source link

Issue with resetting NumberProperty with range #427

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

@matthew-blackman and I ran into a case in Projectile Motion (https://github.com/phetsims/projectile-motion/issues/317) where we changed an initial value of a Property and started getting assertions on reset because the initial value was out of range. We were able to solve it in our sim code with:

Subject: [PATCH] convert all migration rules into new pattern, https://github.com/phetsims/phet-io/issues/1899
---
Index: js/common/model/Target.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Target.ts b/js/common/model/Target.ts
--- a/js/common/model/Target.ts (revision 3427f5fc7454a9cf499610331e082570c9bef06b)
+++ b/js/common/model/Target.ts (date 1671217747180)
@@ -58,7 +58,7 @@
    * Reset these properties
    */
   public reset(): void {
-    this.positionProperty.reset();
+    this.positionProperty.resetValueAndRange();
   }

   /**

and equally as well for our case:

Subject: [PATCH] convert all migration rules into new pattern, https://github.com/phetsims/phet-io/issues/1899
---
Index: js/common/model/Target.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/model/Target.ts b/js/common/model/Target.ts
--- a/js/common/model/Target.ts (revision 3427f5fc7454a9cf499610331e082570c9bef06b)
+++ b/js/common/model/Target.ts (date 1671217787735)
@@ -58,6 +58,7 @@
    * Reset these properties
    */
   public reset(): void {
+    this.positionProperty.rangeProperty.reset(); // the range is inclusive of all possible initial Property values, so set it first
     this.positionProperty.reset();
   }

But then we realized that it is silly that NumberProperty.reset doesn't utilize this behavior by default. I'll take a look and see if we can use resetValueAndRange always.

zepumph commented 1 year ago

Unit tests are passing, I added another test case where it used to fail. Aqua fuzzing for phet and phet-io brand are good to go, and no type errors have been found. I'll check back here to see if CT is clear.

zepumph commented 1 year ago

CT looks good