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

Track nodes snap together but do not connect #384

Closed brroberts1231 closed 6 years ago

brroberts1231 commented 7 years ago

Found while testing phetsims/QA#50 Steps to reproduce: On the playground screen, drag out two pieces of track just a little (not above ground level). They will snap to ground level, and overlap each other. Grab one of them by the center node, and lift it up, then grab the other track by its center node and lift it up. The one endpoint of the second track will snap to overlap the endpoint of the first track, but they will not connect until you click or move one of those endpoints. Also, sometimes moving the tracks will cause gray artifacts to appear as you can see in the gif. r1

This problem does not appear to be specific to platform or sim version. Found initially on 1.4.0-phetiorc.1, but it occurs on the live version on the phet website, and I looked at 1.3.1-dev.6 as well since it was the most recent non io version.

samreid commented 6 years ago

I can't reproduce this in master, but it is likely because the tracks are no longer popping above ground level--this is likely a new bug.

samreid commented 6 years ago

The simplest solution to this problem will create a change in behavior--previously (and in the published version) if you click on a track in the toolbox and release nothing happens. For the proposed fix, if you tap a track in the toolbox and release, it will pop above the track. I suspect it will be fine, but would be good to run it past the design team before publishing this change. @ariel-phet can you comment?

samreid commented 6 years ago

Fix for https://github.com/phetsims/energy-skate-park-basics/issues/384#issuecomment-343788762 committed above and available for testing on phettest.

samreid commented 6 years ago

Also after fixing https://github.com/phetsims/energy-skate-park-basics/issues/384#issuecomment-343788762 I can now easily see the behavior described in https://github.com/phetsims/energy-skate-park-basics/issues/384#issue-261127691. @ariel-phet can you please reproduce that problem and let me know what you recommend for the desired behavior?

samreid commented 6 years ago

@brroberts1231 and/or @phet-steele can you please indicate which browser(s) show the gray artifacts? I haven't been able to see them on Mac/Chrome or Mac/Firefox.

@ariel-phet said the rest of the behavior aside from the gray artifacts is ok, so we'll investigate the gray artifacts a bit more before closing.

samreid commented 6 years ago

Testing the requirejs version, I still do not see the gray artifacts on Mac Chrome. Win10 Chrome and Win10 Edge also show no gray artifacts. @ariel-phet do you want more testing on this before next Dev test?

ariel-phet commented 6 years ago

@samreid, no lets go ahead and close for the moment

jessegreenberg commented 6 years ago

From #205 we may need a different fix for this. @samreid said

if you tap the track in the toolbox 5 times, you will deplete the track from the toolbox--that seems odd.

This commit reintroduced #205: https://github.com/phetsims/energy-skate-park-basics/commit/7b7b1783686a3e17efe93b08db3df3970f075ed1

jessegreenberg commented 6 years ago

Reading this issue more, I see that https://github.com/phetsims/energy-skate-park-basics/commit/7b7b1783686a3e17efe93b08db3df3970f075ed1 was a fix for a bug where tracks were no longer popping above ground.

But in https://github.com/phetsims/energy-skate-park-basics/issues/384#issuecomment-344341800 @ariel-phet said that the behavior in the original issue ticket was OK (or at least acceptable).

jessegreenberg commented 6 years ago

https://github.com/phetsims/energy-skate-park-basics/commit/7b7b1783686a3e17efe93b08db3df3970f075ed1 added

      track.bumpAboveGround();

to trackDragEnded.

I was recently browsing old versions of this code and could have sworn that line used to be further down in the trackDragEnded listener. Ill look around.

jessegreenberg commented 6 years ago

It was in daebed6. This will be fixed if

this.bumpAboveGround;

gets moved into if ( this.startedDrag ) check.

But why was it removed?

jessegreenberg commented 6 years ago

This commit looks like the culprit: 70fa3e4

EDIT: Commit message is "Don't manipulate properties of disposed tracks, #393"

jessegreenberg commented 6 years ago

Fixed in the above commit, the track is bumped above ground only if the track hasn't been disposed and only if drag has started. This issue can be closed again.