Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reference Line's x value should snap to 1 decimal place #305

Closed
pixelzoom opened this issue Mar 22, 2023 · 10 comments
Closed

Reference Line's x value should snap to 1 decimal place #305

pixelzoom opened this issue Mar 22, 2023 · 10 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 22, 2023

For phetsims/qa#921 ...

While working on other issues, I noticed that with "Value" preference on, the Reference Line displays only 1 decimal place for the x value. For example:

screenshot_2462

But inspecting tools.referenceLine.xProperty in Studio, the value is actually 5.496479633520075.

So much like sliders, I believe that we should be snapping to 1 decimal place at the end of a drag cycle. I'm going to go ahead and do this, and confirm later during review.

Other scrubbers (tangent, area) do not need to be snapped, because they do not display the x coordinate.

@pixelzoom pixelzoom self-assigned this Mar 22, 2023
pixelzoom added a commit that referenced this issue Mar 22, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 22, 2023

Ready for review by @amanda-phet. First, confirm that this change is desired. Then confirm that it is working correctly: Open the sim in Studio, select tools.referenceLine.xProperty in the tree, then drag the Reference Line scrubber. Confirm that the xProperty snaps to 1 decimal place when the Reference Line scrubber is released.

@pixelzoom pixelzoom assigned amanda-phet and unassigned pixelzoom Mar 22, 2023
@pixelzoom pixelzoom changed the title Reference Line should snap to 2 decimal places Reference Line should snap to 1 decimal place Mar 22, 2023
@pixelzoom pixelzoom changed the title Reference Line should snap to 1 decimal place Reference Line's x value should snap to 1 decimal place Mar 22, 2023
pixelzoom added a commit that referenced this issue Mar 22, 2023
@veillette
Copy link
Contributor

veillette commented Mar 27, 2023

@amanda-phet stated in a Slack conversation:

Yes, I agree the change is a good one (the lack of precision's of the line actually came up in a meeting I was in Friday!)
... I think it is a good change.

@veillette
Copy link
Contributor

Unassigning @amanda-phet, I'll test that it is working properly in Studio.

@veillette veillette assigned veillette and unassigned amanda-phet Mar 27, 2023
@veillette
Copy link
Contributor

I can confirm that the xProperty in Studio snaps to 1 decimal place when the Reference Line scrubber is released.

@veillette
Copy link
Contributor

I realized that one downside to this behavior is that the snapping is present even when values are not on. As a result, it is not possible to put the reference line at particular point of interest.

A typical usage of the reference line would be to create a curve, and align the reference line at the maximum of f(x) and observe that derivative curve is zero at this point. The maximum of f(x) is unlikely to be at multiplies of 0.1, so it is slightly off the maximum. (see below where one cannot quite get the maximum of the integral ).
image

I see two options

  • (1) Leave as is. It is a very small offset, barely noticeable.
  • (2) Make the snapping conditional: It only snaps the reference line if values are present.

I would vote for two, but I am happy to be outvoted.

@veillette
Copy link
Contributor

I thought it would be an easy issue to review and close 😞. Reassigning to @amanda-phet for input.

@pixelzoom
Copy link
Contributor Author

In today's standup meeting, @amanda-phet and @catherinecarter said that they'd like to revert the snapping, and that it's OK that the displayed value and Property value may be slightly different. I've done that in the above commit.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Mar 28, 2023

So the next question is... When the model value and the displayed value may be different, we typically both the model and displayed value in the PhET-iO API. We currently do not provide a way to get the displayed value.

@amanda-phet please choose one of these 3 options:

(1) Do nothing.

(2) Instrument referenceLineNode.numberDisplay. The Studio tree is shown below. The value will contain a lot of markup, which makes it more difficult for the client to parse, for example:
"<span style='font-family: \"Times New Roman\", Times, serif;font-style: italic'>‪x‬</span> = 5.5"

screenshot_2475

(3) Add new Property referenceLineNode.xDisplayProperty. The Studio tree is shown below. The value will look like (for example): 5.5

screenshot_2476

@amanda-phet
Copy link
Contributor

Option (3) sounds best to me. Thanks for thinking about all of these implications!

pixelzoom added a commit that referenced this issue Mar 28, 2023
@pixelzoom
Copy link
Contributor Author

Option (3) is done, closing. Feel free to have a look in master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants