phetsims / energy-skate-park-basics

"Energy Skate Park: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/energy-skate-park-basics
GNU General Public License v3.0
2 stars 9 forks source link

Tracks can be lost #417

Closed megan-phet closed 6 years ago

megan-phet commented 6 years ago

Test device:

MacBook Pro

Operating System:

macOS 10.13.5

Browser:

All, latest.

Problem description:

For phetsims/QA/issues/134. Moving the tracks to the ground will bump them up off the screen.

This problem is similar to #271, which was found by @jonathanolson.

Steps to reproduce:

  1. Link the tracks together.
  2. Drag the last node to the ground.
  3. Drag the second to last node to the ground.
  4. Alternate dragging nodes to the ground.
  5. Delete last node.

Screenshots:

https://drive.google.com/file/d/1UyvXM0SwB20Nnr4YSo-wjTXSAoc31OGB/view?usp=sharing

Troubleshooting information (do not edit):

Name: ‪Energy Skate Park: Basics‬ URL: https://phet-dev.colorado.edu/html/energy-skate-park-basics/1.4.0-dev.1/phet/energy-skate-park-basics_en_phet.html Version: 1.4.0-dev.1 2018-06-20 00:09:06 UTC Features missing: touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_5) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/11.1.1 Safari/605.1.15 Language: en-US Window: 1264x736 Pixel Ratio: 2/1 WebGL: WebGL 1.0 (2.1 NVIDIA-10.30.33 355.11.10.10.35.101) GLSL: WebGL GLSL ES 1.0 (1.20) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 31 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 16) Max viewport: 8192x8192 OES_texture_float: true Dependencies JSON: {"assert":{"sha":"928741cf","branch":"master"},"axon":{"sha":"f0522e7c","branch":"master"},"brand":{"sha":"89d28f63","branch":"master"},"chipper":{"sha":"e8946524","branch":"master"},"dot":{"sha":"6482f8c9","branch":"master"},"energy-skate-park-basics":{"sha":"288fcefd","branch":"master"},"joist":{"sha":"22e437d5","branch":"master"},"kite":{"sha":"a1086efc","branch":"master"},"phet-core":{"sha":"17326041","branch":"master"},"phet-io":{"sha":"3ea0727a","branch":"master"},"phet-io-wrapper-classroom-activity":{"sha":"53708616","branch":"master"},"phet-io-wrapper-hookes-law-energy":{"sha":"8a546a32","branch":"master"},"phet-io-wrapper-lab-book":{"sha":"1527afd6","branch":"master"},"phet-io-wrappers":{"sha":"8d814eab","branch":"master"},"phetcommon":{"sha":"6ec8cd89","branch":"master"},"query-string-machine":{"sha":"4182612f","branch":"master"},"scenery":{"sha":"88cb642e","branch":"master"},"scenery-phet":{"sha":"7bcde0b2","branch":"master"},"sherpa":{"sha":"88c3b828","branch":"master"},"sun":{"sha":"7579e8fa","branch":"master"},"tandem":{"sha":"8461b6f3","branch":"master"}}
ghost commented 6 years ago

It is possible to lose just one track following the above steps. @megan-phet thought it would be a good idea to show how to lose all of the tracks.

This issue can be resolved by pressing the reset all button.

jessegreenberg commented 6 years ago

This does look related to #271, but easier to reproduce. I do see this in the published version. bumpAboveGround docs are tagged with #71, heading over there for more info.

jessegreenberg commented 6 years ago

Comments over there say

To keep good performance, we could match the Java behavior: constrain the control points to y>0 and bump up splines after release of control point or track.

jessegreenberg commented 6 years ago

While looking into this I noticed that tracks can also be lost when resizing the window: https://i.gyazo.com/1a322e46acf3e976b5da1cdb46d3591d.gif

jessegreenberg commented 6 years ago

One way to fix this is to restrict control points to the available bounds when bumping the track above ground and when the model bounds change. This changes behavior slightly because the track shape can change during some window size changes. @arouinfar @ariel-phet or @samreid can you think of a reason this would not be desirable?

For instance, here is the new behavior: https://i.gyazo.com/828e4e0a215fa35200438908e81dc925.gif

Published version behaves like this: https://i.gyazo.com/20911c3e2bac2b98f4aed98ae67e6f1f.gif

Notice how control points are free to move off screen in the published version. But there are cases where they can get lost as pointed out in the original issue ticket.

samreid commented 6 years ago

I agree the main disadvantage of the proposed approach is changing the shape of the track, but it is impossible to keep the track shape intact and fully in bounds for the cases you described. So maybe that's OK? I'd love to hear from @arouinfar and @ariel-phet.

arouinfar commented 6 years ago

@samreid I'm on board with @jessegreenberg's proposed solution. I think it's important to keep the track nodes within the visible bounds, so shifting the track shape when resizing the window is an acceptable trade-off.

I doubt that many users will be habitually resizing their browser windows. However, for those who do resize the window, I think the behavior in @jessegreenberg's gif looks pretty natural so it's unlikely to be perceived as a bug.

ariel-phet commented 6 years ago

Agreed @jessegreenberg proposed solution seems like a good trade-off

jessegreenberg commented 6 years ago

Great, thanks all. Ready for review.

megan-phet commented 6 years ago

@jessegreenberg it's fixed on master.

jessegreenberg commented 6 years ago

Thanks @megan-phet! Since we are currently in dev testing, we can be sure that the fix will make it into the next RC. Closing this issue.