phetsims / hookes-law

"Hooke's Law" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
1 stars 4 forks source link

Applied Force only reaches +-99N with slider thumb #74

Closed KatieWoe closed 1 month ago

KatieWoe commented 5 years ago

Test device: Dell Operating System: Win 10 Browser: Chrome Problem description: Found during https://github.com/phetsims/QA/issues/309 but unconnected to that release. When moving the arm to push springs in you may be stopped before achieving the maximum force. Seen only under certain circumstances. Steps to reproduce:

  1. Go to the Systems screen
  2. Set the spring to be in series
  3. Make sure the spring constants are low ( Either both 200 N/m or one 200 N/m and one 210 N/m)
  4. Drag the arm to the left as far as possible
  5. Note that the slider for force of the arm will only read -99 N
  6. You can use the slider and/or picker to achieve - 100 N

Screenshots: ninetyninepercent

Troubleshooting information (do not edit):

Name: ‪Hooke's Law‬ URL: https://phet.colorado.edu/sims/html/hookes-law/latest/hookes-law_en.html Version: 1.0.16 2019-04-11 14:52:46 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/73.0.3683.103 Safari/537.36 Language: en-US Window: 1536x722 Pixel Ratio: 2.5/1 WebGL: WebGL 1.0 (OpenGL ES 2.0 Chromium) GLSL: WebGL GLSL ES 1.0 (OpenGL ES GLSL ES 1.0 Chromium) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 4096 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {"assert":{"sha":"11bc6a6d","branch":"HEAD"},"axon":{"sha":"e2237347","branch":"HEAD"},"babel":{"sha":"44cad6ee","branch":"master"},"brand":{"sha":"f7786ac6","branch":"HEAD"},"chipper":{"sha":"c544406e","branch":"HEAD"},"dot":{"sha":"d99cbe46","branch":"HEAD"},"hookes-law":{"sha":"3b63cdb1","branch":"HEAD"},"joist":{"sha":"a7f03638","branch":"HEAD"},"kite":{"sha":"40dc703a","branch":"HEAD"},"phet-core":{"sha":"e9f36af1","branch":"HEAD"},"phetcommon":{"sha":"6ad192fd","branch":"HEAD"},"phetmarks":{"sha":"4e37505a","branch":"HEAD"},"scenery":{"sha":"d88e9e6f","branch":"HEAD"},"scenery-phet":{"sha":"88ade507","branch":"HEAD"},"sherpa":{"sha":"1759eade","branch":"HEAD"},"sun":{"sha":"b66398a5","branch":"HEAD"},"tandem":{"sha":"c54f1082","branch":"HEAD"}}
KatieWoe commented 5 years ago

Also happens on an iPad, so it is not device specific.

pixelzoom commented 5 years ago

@KatieWoe You indicated that this happened as part of https://github.com/phetsims/QA/issues/309, which is a maintenance release involving hookes-law 1.0.17-rc.1. But the details you provided indicate that you were testing version 1.0.16. Please clarify whether this happens in hookes-law 1.0.17-rc.1 and master. I cannot reproduce in master on macOS 10.14.4 with latest Safara, Chrome, or Firefox.

KatieWoe commented 5 years ago

The details were because I tested on the published version to make sure it wasn't due to the maintenance release. It occurs on the published, and those are the details I used. I hadn't checked on master, so its possible it had already been addressed there.

pixelzoom commented 5 years ago

I am able to reproduce this in 1.0.16 (the version currently published on the PhET website) and in 1.0.17-rc.1. I am not able to reproduce in master. @KatieWoe does that match what you see?

KatieWoe commented 5 years ago

It does @pixelzoom

pixelzoom commented 5 years ago

OK. This is likely something related to NumberControl (common code) that was fixed in master. There have been so many changes to NumberControl since the 1.0 release branch that I don't think we should attempt to patch it.

@arouinfar are you OK with this problem existing until we get around to creating a 1.1 release branch for hookes-law? That is unlikely to happen anytime soon, especially with the PhET-iO stuff that needs to be sorted out before this sim can be published from master.

arouinfar commented 5 years ago

@arouinfar are you OK with this problem existing until we get around to creating a 1.1 release branch for hookes-law? That is unlikely to happen anytime soon, especially with the PhET-iO stuff that needs to be sorted out before this sim can be published from master.

Yes, that sounds reasonable to me, given that the current behavior is unlikely to do any pedagogical damage.

pixelzoom commented 5 years ago

We'll keep this issue open until it's regression tested for the next release branch.

To summarize... This problem is present in the 1.0 release branch, but is not present in master. It's likely something that was fixed in NumberControl.

brooklynlash commented 3 years ago

I have noticed that now it goes to -100N, but the arrow is still on, like you can press to go further. If you press the button it then goes unclickable like it should be, but I am not sure if this is a problem or not. Only happens when the spring constants aren't 200N/m and 200N/m, and maybe some other ratio combinations, haven't explored fully yet. hookeslaw

Also, If you increase the spring constants on both springs, it can only go to 99.9N on each side when dragged. Starts at around 250 N/m.

pixelzoom commented 3 years ago

Thanks @brooklynlash. It sounds like you've uncovered a few things that should be investigated. So I'll remove the "fixed-awaiting-deploy" label.

brooklynlash commented 3 years ago

I'll try to observe some patterns in the sim and see if it's replicated on Win10. I'll let you know what I see.

brooklynlash commented 3 years ago

On the Systems screen, with series system:

Applied Force reaches the 99.9N on either side when left spring is 220-240, 290, 310, 320, 350, 390, 430, 460, 530, 560, 570, 580, 590 N/m, while right spring is 200 N/m.

Applied Force reaches 100.0N but the arrow button is still enabled when the left spring is at 210, 250, 270, 280, 300, 330, 340, 370, 380, 420, 440, 450, 480, 490, 510, 520, 540 N/m, and the right spring is 200 N/m.

There are no problems with the slider when the left spring is at 200, 260, 360, 400, 410, 470, 500, 600 N/m while the right spring is at 200 N/m. Haven't tried with the other side yet.

It is hard to find a pattern with this observation, so let me know if you want me to check anything else.

pixelzoom commented 3 years ago

Thanks @brooklynlash. Good info. I'll let you know if I have any questions. There are no immediate plans to re-publish this sim, so I may not get to this for awhile.

pixelzoom commented 3 years ago

I had a few minutes to look at this today. I'm on macOS 11.3.1 with Chrome 91, Safari 14.1, and Firefox 89. @brooklynlash didn't specify here configuration.

I can't reproduce what @brooklynlash described in https://github.com/phetsims/hookes-law/issues/74#issuecomment-840033575 and https://github.com/phetsims/hookes-law/issues/74#issuecomment-840140864. The Applied Force slider range is [-100.0,+100.0], and the arrow buttons enable/disable correctly.

I'm wondering if this is specific to Windows, or if a common-code change corrected this in the 2 months since it was reported. I'll leave this open, and either have QA retest when they have time, or revisit when work on this sim resumes.

pixelzoom commented 3 years ago

I'll leave this open, and either have QA retest when they have time, or revisit when work on this sim resumes.

Slack dev-public channel:

Kathryn Woessner 11:26 AM QA repo could use a test or two if anyone has something

Chris Malley 11:27 AM Please see if you can reproduce https://github.com/phetsims/hookes-law/issues/74 in master.

brooklynlash commented 3 years ago

@KatieWoe @pixelzoom Looks fixed in master, Win10 Chrome.

pixelzoom commented 3 years ago

Excellent! But weird - I can't guess how this might have self-healed in common code. Anyway... Thanks for testing, and I'm going to close this.

KatieWoe commented 2 months ago

I'm seeing this problem again in https://github.com/phetsims/qa/issues/1102. It now only happens when dragging the claw, rather than the slider itself.

https://github.com/phetsims/hookes-law/assets/41024075/7cd1ff01-8850-4887-8b22-1c527071a288

pixelzoom commented 2 months ago

Reproduced in main, as in the video that @KatieWoe provided in https://github.com/phetsims/hookes-law/issues/74#issuecomment-2197604438. Dragging the "claw" fully to the right results in 100.0 N, but dragging fully to the left only results in -99.9 N.

pixelzoom commented 2 months ago

This is only a problem with series springs in the Systems screen (see SeriesSystem.ts), which is the case demonstrated by @KatieWoe in https://github.com/phetsims/hookes-law/issues/74#issuecomment-2197604438.

I do not see the problem in the Intro screen or in the Systems screen with parallel springs.

pixelzoom commented 2 months ago

The problem is with the value of RoboticArm leftProperty. When the "claw" is dragged fully to the left, the value should be 0.5, but it is 0.501. This is probably due to rounding error, and it's responsible for applied force being computed as -99.9 N. Here's the relevant code in RoboticArmNode, associated with its DragListener:

        // constrain to multiples of a specific interval, see https://github.com/phetsims/hookes-law/issues/54
        if ( options.displacementInterval ) {
          left = Utils.roundToInterval( left, options.displacementInterval );
        }
        left = Utils.toFixedNumber( left, HookesLawConstants.DISPLACEMENT_DECIMAL_PLACES );

This change in HookesLawConstants.ts resolves the problem:

-  DISPLACEMENT_DECIMAL_PLACES: 3,
+  DISPLACEMENT_DECIMAL_PLACES: 2,

But in https://github.com/phetsims/hookes-law/issues/54 we decided that we wanted 3 decimals places for displacement values. And this may affect other things.

Also... Inspecting the value of rightSpring.rightRangeProperty (leftRangeProperty in RoboticArmNode), it appears to be slightly off: [0.5005, 1.5005]. I suspect that it should be [0.5, 1.5].

pixelzoom commented 2 months ago

Fixed in https://github.com/phetsims/hookes-law/commit/c766c21cae2495c292b593042f976976a7b3790b. This was hard to find, but trivial to change. For 2 springs in series, instead of passing rightSpring.rightRangeProperty, we should be passing equivalentSpring.rightRangeProperty to RoboticArmNode.

@KatieWoe please verify, close if OK.

KatieWoe commented 2 months ago

Looks good on main!

Nancy-Salpepi commented 2 months ago

This issue is still present in 1.1.0-rc.1.

Steps:

  1. On the Systems Screen, select the Spring Series option
  2. Change the values of the springs to max values (600 and 600)
  3. Grab the robotic arm and try to reach min/max Applied Force values

https://github.com/phetsims/hookes-law/assets/87318828/0ff9f580-0fff-455b-839d-c1a53967ec84

Nancy-Salpepi commented 2 months ago

I also see the issue when the springs are in parallel:

https://github.com/phetsims/hookes-law/assets/87318828/1c6cd25c-000e-4e42-81f7-7164381c7bab

pixelzoom commented 2 months ago

Reproduced in master on Systems screen with series and parallel systems. The zombie issue that refuses to die...

pixelzoom commented 2 months ago

Also reproduced on the Intro screen:

  1. Start the sim, go to the Intro screen.
  2. Set "Spring Constant 1" to 390 N/m.
  3. Drag the claw through it's full range. Note that Applied Force only varies from [-99.8,+99.8].
pixelzoom commented 2 months ago

There are several related Properties that affect each other and can be adjusted independently.

Spring implements F = kx (Hooke's Law) with 3 Properties, all of which can be adjusted in the UI:

Spring also has rightRangeProperty, which affects the range of RoboticArm leftProperty, which is used by each system model (e.g. SingleSpringSystem.ts) to set a Spring's displacementProperty.

rightRangeProperty is responsible for the problem with being able to get the correct min/max for Applied Force when dragging the claw. Below is the derivation of rightRangeProperty in Spring.ts. Depending on the value of springConstant, the range will have more or less floating-point error, and (while dragging the claw) the error will propagate back to computing the value for appliedForceProperty.

    if ( options.appliedForceRange ) {
      this.rightRangeProperty = new DerivedProperty(
        [ this.springConstantProperty, this.equilibriumXProperty ],
        ( springConstant, equilibriumX ) => {
          const minDisplacement = this.appliedForceRange.min / springConstant;
          const maxDisplacement = this.appliedForceRange.max / springConstant;
          return new Range( equilibriumX + minDisplacement, equilibriumX + maxDisplacement );
        } );
    }
pixelzoom commented 1 month ago

I finally found the pesky needle in the haystack.

This problem was introduced way back in https://github.com/phetsims/hookes-law/issues/54, where we wanted the Displacement slider and "claw" in the Energy screen to be constrained to changing displacement in 0.05 m increments. The view was incorrectly truncating the positon of the claw for all screens, resulting in a loss of precision for displacement, and therefore an inaccurate derivation of Applied Force.

Here's the relevant code and fix in RoboticArmNode.ts:

        // constrain to multiples of a specific interval, see https://github.com/phetsims/hookes-law/issues/54
        if ( options.displacementInterval ) {
          left = Utils.roundToInterval( left, options.displacementInterval );
        }
-       left = Utils.toFixedNumber( left, HookesLawConstants.DISPLACEMENT_DECIMAL_PLACES );

In the first 2 screens, the model will now have the correct (full precision) value for the claw's position, will compute the correct applied force, and the view will (elsewhere) handle display of displacement and applied force with the desired number of decimal places.

@Nancy-Salpepi please review. Close if OK.

pixelzoom commented 1 month ago

Fix was pushed to main and 1.1 branches in the above commits.

Nancy-Salpepi commented 1 month ago

Looks fixed on main!

@Nancy-Salpepi please review. Close if OK.

@pixelzoom Are we not doing an RC spot check?

pixelzoom commented 1 month ago

@pixelzoom Are we not doing an RC spot check?

Ah, right you are. Leaving this open for verification in 1.1.0-rc.2.

KatieWoe commented 1 month ago

Linking to https://github.com/phetsims/qa/issues/1104 for tracking purposes.

pixelzoom commented 1 month ago

Ready for verification in https://github.com/phetsims/qa/issues/1112. Please close if OK.

Nancy-Salpepi commented 1 month ago

Looks great in rc.2! Closing.