pixelzoom
commented
3 years ago Discussed at 2/4/21 design meeting. @DianaTavares is going to provide mockups showing where the time control should appear, and how it should look.
Closed pixelzoom closed 3 years ago
pixelzoom
commented
3 years ago Discussed at 2/4/21 design meeting. @DianaTavares is going to provide mockups showing where the time control should appear, and how it should look.
DianaTavares
commented
3 years ago This is the mockup:
and this how it looks in the full simulation, screen 1 and 2:
pixelzoom
commented
3 years ago Something that @issamali67 and I discussed on Slack that should be captured here.
OneDimensionModel and TwoDimensionsModel both define this "speed" like this:
// @public {Property.<number>} determines the speed at which the sim plays
this.simSpeedProperty = new NumberProperty( OneDimensionConstants.INIT_SPEED, {
tandem: tandem.createTandem( 'simSpeedProperty' ),
range: new Range( OneDimensionConstants.MIN_SPEED, OneDimensionConstants.MAX_SPEED )
} );TimeControlNode specifies it's speed option like this:
// {EnumerationProperty.<TimeSpeed>|null} - Play speed Property for the radio button group. If null,
// no radio buttons included in this control.
timeSpeedProperty: null,Because they are different types, this will not work:
new TimeControlNode( ..., {
...
timeSpeedProperty: model.simSpeedProperty
} );So in addition to using TimeControlNode in the view, you'll need to adjust the model. Here's one way to do that (untested, but the right direction).
As shown in the mockups, we need to support two speeds, "Normal" and "Slow". In NormalModesConstants.js, add these constants. They are the multipliers that will be applied to dt in the model's step method.
NORMAL_SPEED: 1,
SLOW_SPEED: 0.2,Then in OneDimensionalModel.js, define a new timeSpeedProperty that you'll pass to TimeControlNode:
// @public used by the time control to select a speed
this.timeSpeedProperty = new EnumerationProperty( TimeSpeed, TimeSpeed.NORMAL, {
validValues: [ TimeSpeed.NORMAL, TimeSpeed.SLOW ]
} );Also in OneDimensionalModel.js, change simSpeedProperty to be derived from timeSpeedProperty:
// @private {DerivedProperty.<number>} multiplier for dt used in step
this.simSpeedProperty = new DerivedProperty(
[ this.timeSpeedProperty ],
timeSpeed => ( timeSpeed === TimeSpeed.NORMAL ) ? NormalModesConstants.NORMAL_SPEED : NormalModesConstants.SLOW_SPEED
);Unfortunately you'll need to duplicate whatever you do in both OneDimensionalModel.js and TwoDimensionalsModel.js. There's a lot of duplication there that needs to be factored out, see https://github.com/phetsims/normal-modes/issues/63.
And these constants, duplicated in OneDimensionalConstants.js and TwoDimensionalConstants.js, should be reviewed. Any of them that are no longer used should be deleted.
MIN_SPEED: 0.02,
INIT_SPEED: 1,
MAX_SPEED: 3,
DELTA_SPEED: 0.1,EDIT: You could also do away with simSpeedProperty, and make this change in singleStep:
- dt *= this.simSpeedProperty.get();
+ dt *= ( ( timeSpeed === TimeSpeed.NORMAL ) ? NormalModesConstants.NORMAL_SPEED : NormalModesConstants.SLOW_SPEED );@issamali67 contacted me on Slack and said:
Including
this.simSpeedProperty = new DerivedProperty(…)in both 1d and 2d models generates a “assert.js:21 Assertion failed: linear requires a number to evaluate” error.I assume I need to comment out the original this.simSpeedProperty = new Enumeration(…), which I did.
I think the problems is in
singleStep. Thethis.simSpeedProperty.get()is unable to get the ‘timeSpeed’ and dt is not getting a number.I also tried the one liner step. Somehow I need to grab timeSpeed from the ‘new’ construct.
I replied:
I'm afraid I can't debug that without a stack trace. I spent a couple of minutes and got the approach that I described to work. I'll put a patch in the GitHub issue. Perhaps you can inspect that patch and figure out what you're doing wrong. If not, let's set up a time to pair program.
Here's that patch:
And here's a screenshot showing the TimeControlNode in the One Dimension screen:
issamali67
commented
3 years ago I think the difference between what @pixelzoom did and what I did is that @pixelzoom took out this.simSpeedProperty and defined another thisTimeScaleProperty . I tried instead to do the changes Chris suggested earlier in this.simSpeedProperty itself. And this did not work which puzzled me. I tried to find if this.simSpeedProperty is used somewhere else, but could no. So I am still puzzled!
jessegreenberg
commented
3 years ago The API for StepButton changed and isPlayingProperty was replaced with enabledProperty. When TimeControlNode is used in this sim the setting of enabledProperty can be removed because it is the default behavior for TimeControlNode.
pixelzoom
commented
3 years ago For https://github.com/phetsims/qa/issues/724, @ariel-phet requested this issue for the Prototype.
pixelzoom
commented
3 years ago This is ready for review in master by @DianaTavares and @ariel-phet. Screenshots are shown below.
@DianaTavares the mockups that you provided in https://github.com/phetsims/normal-modes/issues/58#issuecomment-776191959 do not match the layout of the control panel in the sim, and are missing UI components. And in one mockup, you show the time control in a separate panel, which is not possible because the 2 screens share the same control panel. So I simply moved the time control to the bottom of the control panel, with a separator above it. All other controls are where they were when I started this work.
@DianaTavares and @ariel-phet Let me know if we need to adjust the "slow" speed. It currently scales to 20% of normal speed.
ariel-phet
commented
3 years ago @pixelzoom I reviewed on master -- looks great, nice placement and certainly cleans things up and brings the sim to the modern standard. The slow speed seems appropriate. Everything now looks buttoned up for the porotype. Closing.
Related to #57...
Normal Modes currently has a "Speed" control that looks like the one in the legacy (Flash) version:
PhET's HTML5 sims have standardized on a general time control. It's TimeControlNode, and it lives in the scenery-phet repository. Run the scenery-phet demo (via phetmarks), go to the Components screen, choose TimeControlNode from the combo box, and you'll see some examples:
normal-modes needs to be converted to TimeControlNode. That involves:
This would be a good first task for @issamali67. Consult with @arouinfar and @ariel-phet on design questions. Feel free to consult with me on TimeControlNode options and implementation issues.