Identify fields that may need to be stateful

phetsims / keplers-laws

"Kepler's Laws" is an educational simulation in HTML5, by PhET Interactive Simulations.

GNU General Public License v3.0

1 stars 1 forks source link

Identify fields that may need to be stateful #209

Closed AgustinVallejo closed 10 months ago

AgustinVallejo commented 11 months ago

Related to https://github.com/phetsims/keplers-laws/issues/206 I'll be editing this comment with new features to instrument.

[x] Stopwatch reset button (11/16: We don't need to include this)
[x] selectedLawProperty
[x] (D) Zoom level kinda starts in the middle of the animation when testing standard wrapper
[x] All the physical properties included in the engine should be instumented and grouped together.
[x] (D) Everything in MoreOrbitalData
[x] (D) The result of T/a
[x] hasUserInteractedProperty
[x] #214
[x] Drawn line in third law

I'm noting with (D) the ones which are derived properties.

AgustinVallejo commented 11 months ago

Although looking at MoreOrbitalDataPanel.ts it seems everything showed in there is derived from the planet's position and velocity properties. Should we still include those? @arouinfar

AgustinVallejo commented 11 months ago

As for the zoom issue, we're going to use isSettingPhetioStateProperty in the zoomLevelProperty.link to avoid doing the animation

AgustinVallejo commented 11 months ago

The initial list of things that needed instrumentation has been covered. Marking the issue ready for review, and please add anything missing instrumentation.

pixelzoom commented 11 months ago

@AgustinVallejo This is not typically something that the designer can review, because they don't see the internals of the code. If there's something that's not stateful that should be (or something that should not be executed when state is being restore), then the State Wrapper will fail or behave incorrectly. And that seems to be happening in #215. So I suspect that this issue is not yet complete.

pixelzoom commented 11 months ago

Assigning myself, because this is something that another developer (me :) can review.

pixelzoom commented 11 months ago

From duplicate https://github.com/phetsims/keplers-laws/issues/201, here are a few more fields that might need to be stateful:

// KeplersModel
  public resetting = false;

// PeriodTracker
  public beganPeriodTimerAt = 0;
  public trackingState: TrackingState;
  public afterPeriodThreshold = false; // Whether the body has passed some percentage of the period

// PeriodTrackerNode
  public orbitScale = 1;
  public orbitCenter = Vector2.ZERO;
  public radiusX = 1;
  public radiusY = 1;

pixelzoom commented 10 months ago

PeriodTracker and PeriodTrackerNode were addreessed in #222.

@AgustinVallejo and I inspected KeplersModel resetting and didn't see any need for it to be stateful, or any related problems.

So closing this issue.