Add a range for Score.targetXProperty

[zepumph]

No description provided.

[zepumph]

After some more investigation, it looks like this could benefit from having a dynamic range, just like in phetsims/gravity-force-lab#172.

This is because the range is based on the current zoom (in view coords). I will investigate supporting this and report back.

[zepumph]

In the above commits I added a range to the targetXProperty. This is dependent on the current transform, and so is set by the view.

In general this is an excellent improvement for studio. Another bonus we got was a behavior change where now the score target won't get "lost" off to the right of the screen when you zoom out, move it far away from the cannon, and then zoom back in. I set it up to clamp back within the draggable area of the score.

@arouinfar would you please review these two pieces.

1. The behaviour in any brand of the score target and how it stays on the screen always.
2. In studio, look at scoreTargetXProperty, and how you can move it with the slider now. That is based on its sibling scoretargetXRangeProperty. Does this look as desired in studio? Do you think that scoreTargetXProperty should be phetioFeatures? (I know we haven't had that conversation just yet about that).

Let me know what you think!

[zepumph]

http://phettest.colorado.edu/phet-io-wrappers/studio/?sim=friction&phetioValidateTandems&phetioDebug&phetioElementsDisplay=all so you have it.

[zepumph]

As a note, @samreid, @chrisklus @zepumph all discussed the changes to NumberProperty. We noticed that when updating the rangeProperty, you need to make sure the current value of the Property is within that range otherwise you will error out when changing the range. In PM this meant adding this line before setting the range:

      this.targetXProperty.set( Util.clamp( this.targetXProperty.value, newRange.min, newRange.max ) );

We thought about having an option in NumberProperty, like

        // if false, error out, otherwise, when the range Property changes the value, 
        clampValueOnRangeChange: false

but didn't feel like it was perfectly in line with how axon Propeties tend to work, and fail validation. It felt too graceful. Also, what is the difference between that, and just clamping on setValue also (which it felt clear wasn't a good thing to do)?

No action steps here, just a note of a review discussion.

[zepumph]

• Another note, this change is failing fuzzing, especially on the state wrapper, with "value failed isValidValue". It always occurs with both screens at different MVT zooms.
  

Note it is from the downstream sim.

@arouinfar this issue is still assigned to you to just discuss the last part of #192 (comment).

[arouinfar]

The behaviour in any brand of the score target and how it stays on the screen always.

Behavior's looking great!

In studio, look at scoreTargetXProperty, and how you can move it with the slider now. That is based on its sibling scoretargetXRangeProperty. Does this look as desired in studio? Do you think that scoreTargetXProperty should be phetioFeatures? (I know we haven't had that conversation just yet about that).

I think this is looking good in studio. I would not feature the score.targetXRangeProperty, but I think we would want to feature score.targetXProperty. Since this isn't a common component, my guess is that it's best to defer featuring until overrides, but I'll defer to @zepumph.

[zepumph]

That sounds like a great plan. I added a checkbox to your comment in #219. Closing

[zepumph]

Ooops, I still see a checkbox for me to investigate.

[zepumph]

This is easily reproducible.

• state wrapper
• increase the state setting time with ?numberOfMillisecondsBetweenUpdates=4000
• set the bottom state to be all the way zoomed in and move the target to under the cannon.
• Then in in between one cycle zoom all the way out and move the target all the way left.

The state wrapper will try to set the NumberProperty before its range has been updated to the new, larger Range, and so it fails out in NumberProperty.assertNumberPropertyValidateValue.

I'm unsure how to handle this. Perhaps I will bring it to the other PhET-iO devs to see.

[zepumph]

The state setting bug was fixed by treating the range and value of a NumberPropertyIO as one atomic operation during setValue of the state set. This is now fixed and can be closed.

[zepumph]

I wonder if work over in phetsims/axon#274 has changed how I should implement this. Perhaps it would be better for this Property to be nested underneath the NumberProperty.

[zepumph]

The rangeProperty was nested in the above commit. Closing

[arouinfar]

@zepumph when reviewing the sim in studio for #244, I saw some odd behavior with score.targetXProperty.rangeProperty. Unfortunately, it seems to be inconsistent, and I'm not sure how to reliably reproduce.

Twice now, I've seen the current value of projectileMotion.introScreen.model.score.targetXProperty.rangeProperty show up as [-100, 100], instead of the correct range for the default/current zoom level.

We'd also like to remove the slider from studio interface for score.targetXProperty since it adjusts in finer increments than the target within the sim.

[zepumph]

Good find. Because the range is based on the modelViewTransform (in the view), but is created in the model (before the view is created), we need a bogus initial value to fill the range to start. I made this [-100,100]. This is not ideal, but wasn't buggy expect that I wasn't re-calculating based on the transform on reset.

Above fixes this because it recalculates the range on reset. Please review.

[arouinfar]

I haven't been able to reproduce in master, so closing.