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

Moving both hands at near the same time can cause an erroneous in-proportion state #360

Closed
Tracked by #617
phet-steele opened this issue Feb 23, 2021 · 14 comments
Closed
Tracked by #617
Assignees
Labels
type:bug Something isn't working

Comments

@phet-steele
Copy link

Maybe similar to #354?

  1. Enter the Create screen. Make sure sound is on and you can hear it!
  2. Turn on the numbered ladder lines, just to make this easier to see.
  3. Keep the default ratio of 1:2, but this should break with any ratio.
  4. Enter keyboard nav, and tab until you are controlling both hands simultaneously.
  5. Throughout this issue, I will ask you to do a "double move", and that means pressing W + ⬆️ at the same time (for a double up), or by pressing S + ⬇️ at the same time (for a double down), effectively moving both hands at as close to the same time as possible.
  6. Start by doing a double down. You would have started at 2/4, and should now be at 1.5/3.5. If you listen closely, you should hear an erroneous in-proportion sound while the hands are in motion, I'm guessing either from rounding, or an intermediate value that is in-proportion.
  7. At this point, you can push this bug a bit further, but it isn't the easiest. Starting from 1.5/3.5, do a double up with a very quick double down right after. If the bug happened (keep trying if it didn't happen), then you will get a few bugged things.
  • You will hear the sound for "moving a locked ratio" (I don't know what it is called)
  • The background will be in-proportion green, with the Lock Ratio checkbox enabled, even though 1.5/3.5 is not equal to the desired ratio of 1:2.
  • If you click "Lock Ratio" now, you should see the left hand jump to the correct position of 1.75.

You can see the incorrect background and check box in the first screenshot, while the second screenshot is the expected behaviour.
Screen Shot 2021-02-23 at 4 26 13 PM
Screen Shot 2021-02-23 at 4 26 26 PM

Seen on macOS 10.13.6 Chrome and Safari. For phetsims/qa/issues/609.

Troubleshooting Information

URL: https://phet-dev.colorado.edu/html/ratio-and-proportion/1.0.0-rc.2/phet/ratio-and-proportion_all_phet.html
Version: 1.0.0-rc.2 2021-02-13 05:25:44 UTC
Features missing: applicationcache, applicationcache, touch
Flags: pixelRatioScaling
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/88.0.4324.192 Safari/537.36
Language: en-US
Window: 1920x1031
Pixel Ratio: 2/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: 32 uniform: 1024
Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 80)
Max viewport: 16384x16384
OES_texture_float: true

@zepumph
Copy link
Member

zepumph commented Feb 24, 2021

Start by doing a double down. You would have started at 2/4, and should now be at 1.5/3.5. If you listen closely, you should hear an erroneous in-proportion sound while the hands are in motion, I'm guessing either from rounding, or an intermediate value that is in-proportion.

This to me is not a bug, because we can only detect a single keypress at a time. Thus we will have to get either the left or right hand event first. Let's say first it is the left hand, then next it is the right hand. Thus, there are three discrete states in the single threaded UI of the simulation, which the sound is responding to. First you start at 2:4, in proportion; next you are at 1.5:2, out of proportion; next you are at 1.5:3.5, in proportion again and the sound will trigger as you enter proportion here.

The second bug is quite legit. It is because "movingInDirection" is property (lowercase) of the model, which determines what the in-proportion threshold value for fitness. This is not a Property (capitalized), and thus, can't currently be listened to. Thus, when it changes, the view doesn't update. Good find! I will work on this.

@zepumph
Copy link
Member

zepumph commented Mar 1, 2021

By turning RAPRatio.movingInDirection() and RAPModel.inProportion() into Properties, we can not listen to those changes, so that the above incorrect state never occurs. Unfortunately this yields bit of a strange behavior because of the delay in calculating "movingInDirection", the half second or so it takes to stop being in the "moving in direction" state is now shown with the color and locked checkbox enabled. This is by all accounts less buggy and more clear, it is just perhaps a louder demonstration about how our moving in direction algorithm is weird.

I will mark this for design meeting tomorrow to confirm that we are alright with this behavior.

Steps to reproduce:

  1. load rap, either screen
  2. both hands interaction
  3. be in-proportion
  4. move both hands down at the same time
  5. colors will be strange and off, then 1/2 second later it will kick back into the lighter green (out of proportion).

@zepumph
Copy link
Member

zepumph commented Mar 1, 2021

I just met with @terracoda and @BLFiedler, and we think it is worth exploring the patch in #338 (comment) but to make it harder for keyboard instead of worse for touch.

We feel like discrete operations from the keyboard do not require the moving-in-proportion sound as much as the continuous rate/touch interaction does. We will try making keyboard presses harder to create the moving in proportion sound.

@brettfiedler
Copy link
Member

brettfiedler commented Mar 1, 2021

Just to document this rationale - we were pleasantly surprised by the ability to get the moving in proportion sound with key presses when the moving-in-proportion threshold was developed, but had not expected it would necessarily be true. Now that we are against an obstacle that requires us to choose tradeoffs for the experience at the cost of possibly degrading the ability of the normal learning goals of the sim (exploring the space and finding in-proportion states), we are going to try to go with the option that values what the moving-in-proportion threshold was originally intended to do.

zepumph added a commit that referenced this issue Mar 1, 2021
@zepumph
Copy link
Member

zepumph commented Mar 1, 2021

Alright above I did a couple of things:

  • Change the algorithm for what each term's velocity is, requiring three values for comparison instead of just 2 within 30 frames (~.5 seconds). This number can change, but the larger it gets, the laggier it is for touch/tangible input, and the shorter it is, the harder it is to trigger the "moving in proportion" state for keyboard. With 30 it is basically impossible for me to consistently get the choir sound by using the both hands interaction's key presses.
  • A minor refactor in RAPRatio that encapsulated the velocity-tracking code for each term into its own type.

To see just this behavior change, play with https://phet-dev.colorado.edu/html/ratio-and-proportion/1.1.0-dev.3/phet/ratio-and-proportion_en_phet.html.

I also:

Please try this behavior out on phettest.

I'm excited to discuss further tomorrow!

zepumph added a commit that referenced this issue Mar 2, 2021
zepumph added a commit that referenced this issue Mar 2, 2021
…nstead of a couple samples evenly along the way), #360
@zepumph
Copy link
Member

zepumph commented Mar 2, 2021

Found that this change effects the way that keyboard input interacts with the ratio when locked. This caused me to increase the step counter to 40 instead of 30.

I actually found this to be too laggy for all input (including keyboard), so I put it back to 30 frames in 387abc7, d138b09 was a much more helpful fix, where instead of linearly sampling every 3 times per 1/2 second, we now keep track of every single value on every step, and then only check to see if there are 3 distinct values when we calculate velocity. Thus I feel like this is now quite good. To put it simply, d138b09 made it substantially easier to trigger the moving-in-proportion sound with the ratio locked with keyboard. As well as a bit easier to use discrete, both-hands keypresses to accomplish this, though it is not quite consistent or as easy as before. @BLFiedler @terracoda take this for a test drive and let me know how it goes.

Main points for comparison:

  • Lock the ratio, and moving one hand with the keyboard, make sure you don't have press too fast to get the moving-in-proportion sound. Note that this will be the same speed needed to use the slider on iOS + VO to get that sound.
  • If you can use touch and make sure that the moving-in-proportion sound feels adequately responsive to your movement, and not too delayed. I think that us having the value of 30 like we do on master instead of 40 (what I tried for 10 minutes above) for STEP_FRAME_GRANULARITY will be good for this.
  • Note that if you try really hard (at least I had to try really hard), you can still get the moving-in-proportion sound with the both hands interaction by using discrete steps quickly that match the ratio (default left press up 1 for every right press up 2). It is still possible though.

@zepumph
Copy link
Member

zepumph commented Mar 2, 2021

During design meeting we liked what was in place, but felt like the new algorithm shouldn't apply to the ratio when locked. Above I added a workaround so that when locked, you don't need three different values to calculate velocity.

@terracoda @BLFiedler please sign off on this after playing with it in https://phet-dev.colorado.edu/html/ratio-and-proportion/1.1.0-dev.5/phet/ratio-and-proportion_en_phet.html. Unassign as a check off. From here I can move forward with then next RC.

@zepumph
Copy link
Member

zepumph commented Mar 3, 2021

Sounds great. I think one review is plenty. Thanks all for a productive solve!

@phet-steele
Copy link
Author

All seems fixed for me in 1.0.0-rc.3. 😅

@phet-steele
Copy link
Author

Okay actually nevermind! I think I still see this bug happening, but @zepumph let me know if what you are about to read is something totally different or not:

  1. "Create" screen
  2. Set challenge ratio to 1:1
  3. Do NOT select "Lock Ratio"
  4. Use keyboard nav until you are moving both hands simultaneously
  5. Rapidly move between three jumping number keys (for example, I was rapidly pressing 6>5>4 >6>5>4 over and over)

Here, I heard the angelic choir sound repeatedly, even though I was not in a locked ratio.

@zepumph
Copy link
Member

zepumph commented Mar 4, 2021

even though I was not in a locked ratio.

It is quite possible to trigger the angelic (moving-in-proportion) sound without having the ratio locked, just like when using two fingers on touch. That sound is meant to indicate that you are correctly moving both hands and maintaining the ratio throughout the movement. So good work! You can also get it (although it is hard) by setting ratio to 1:2, and the quickly pressing "w" for every time you press "up arrow" twice, thus maintaining the ratio through movement.

this bug happening

I feel like the bug was that the visuals could become out of sync with the model in some cases. I am not able to see that anymore, even in your above case.

@zepumph zepumph assigned phet-steele and unassigned zepumph Mar 4, 2021
@phet-steele
Copy link
Author

I feel like the bug was that the visuals could become out of sync with the model in some cases. I am not able to see that anymore, even in your above case.

@zepumph I should have been more clear! Yes, that is the main part of the bug and I do not see that anymore. However, in part of the bug report I mentioned:

You will hear the sound for "moving a locked ratio" (I don't know what it is called)

I mentioned this because it did not seem correct to hear that at the time. However, you have said otherwise, so I concede there! 😅 BUT, what I have stated in #360 (comment) still seems like a bug to me. Maybe a separate issue at this point? It seems like a bug to me because moving with 1:1 with the jump keys sometimes does make the angelic sound, and often times doesn't. So why the two different behaviours when doing the same thing?

@phet-steele phet-steele assigned zepumph and unassigned phet-steele Mar 4, 2021
@zepumph
Copy link
Member

zepumph commented Mar 9, 2021

So why the two different behaviours when doing the same thing?

It is likely chocked up to being a time-based algorithm. If you do two sets of actions, but one is a bit slower, the slower one likely won't yield this sound. I get the sound pretty consistently if I note the time I do it in. The sound itself is meant to represent a change-over-time relationship. So this also doesn't seem like a bug to me. If it still doesn't seem like I'm understanding you, then let's meet up and talk!

@zepumph zepumph assigned phet-steele and unassigned zepumph Mar 9, 2021
@zepumph
Copy link
Member

zepumph commented Mar 9, 2021

@terracoda, @BLFiedler, @zepumph discussed this today during RAP standing meeting. We feel like the instructions in #360 (comment) produce a case that is not optimal, but that behavior is a result of lots of design work to make a few other cases yield the moving-in-proportion much better (decided during meeting noted #360 (comment)). For example making it easy to hear the moving-in-proportion sound when the ratio is locked (for screen reader use).

We are ready to close this as a team. Thanks @phet-steele for all your hard work. We really appreciate it.

@zepumph zepumph closed this as completed Mar 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants