phetsims / resistance-in-a-wire

"Resistance in a Wire" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/resistance-in-a-wire
GNU General Public License v3.0
1 stars 4 forks source link

Slider thumbs jump during drag #85

Closed samreid closed 7 years ago

samreid commented 7 years ago

After clicking on a slider thumb, then first drag event causes the knob to jump vertically by some 50 pixels or so. This is a new bug, possibly introduced during recent refactoring. The thumb should move continuously without a large discrete jump at the beginning.

@zepumph can you take a look?

Discovered in #79 and will need to be fixed for #77

veillette commented 7 years ago

FYI, the slider was replaced by HSlider but with a custom Thumb.

    // Thumb for the slider
    var thumb = new Image( thumbImage, { rotation: Math.PI / 2, tandem: tandem.createTandem( 'thumb' ) } );
    thumb.scale( ResistanceInAWireConstants.THUMB_HEIGHT / thumb.height );
    thumb.touchArea = thumb.localBounds.dilated( 30 );

    var slider = new HSlider( property, range, {
      trackFillEnabled: 'black',
      rotation: -Math.PI / 2,
      trackSize: new Dimension2( ResistanceInAWireConstants.SLIDER_HEIGHT - 2 * thumb.height, 4 ),
      thumbNode: thumb,
      x: 0,
      centerY: sliderCenterY,
      tandem: tandem.createTandem( 'slider' )
    } );
zepumph commented 7 years ago

I found out the cause of this. It is because of the switch to HSlider.js. In general this is a great refactor, it simplified code and made things work well. Unfortunately there is just one problem. Because this is an 'H' slider, it has some places in the code that it assumes horizontal motion versus vertical. For instance with this bug. See here in the drag started listener. . .

      start: function( event, trail ) {
        if ( self.enabledProperty.get() ) {
          options.startDrag();

          var transform = trail.subtrailTo( self ).getTransform();
          clickXOffset = transform.inversePosition2( event.pointer.point ).x - thumb.x;
        }
      },

Notice how the offset is hard coded to use x values. That is translating into a Y variation (bug) because of the rotation.

samreid commented 7 years ago

Are the vertical sliders in Color Vision rotated HSliders? Why do they not exhibit this problem?

zepumph commented 7 years ago

The reason they don't is because they don't pass in a 'thumb' node, but rather create the thumb in the HSlider, so their 'thumb.x' is 0.

samreid commented 7 years ago

If I omit the thumb option in RIAW, then the bug goes away. Perhaps we just need to center the new thumb properly?

samreid commented 7 years ago

I tried passing

thumbNode: new Node({children:[thumb],x:0,y:0}),

to normalize the thumbNode, but it suffered from the same problem. It looks like this has the same problem too:

thumbNode: new Node({children:[thumb],centerX:0,centerY:0}),
zepumph commented 7 years ago

I think it is more than that. I think that it is because this is desired functionality. In a horizontally positioned slider, if you click on a the right side of a very long thumb image, it makes sense that it would jump to that position on the slider based on the width of the node.

veillette commented 7 years ago

@ariel-phet mentionned some time ago that he would welcome a refactor of these sliders with our generic thumb

samreid commented 7 years ago

In my opinion, the generic thumb looks fine for this sim:

image

In other cases (other sims) we need to support different thumbs so it would be good to (eventually?) get to the bottom of this if it is a flaw in HSlider.

zepumph commented 7 years ago

I seems reasonable to me to use these generic sliders, but @ariel-phet or a designer will have to have the final say. I also think it seems reasonable to me for HSlider to handle vertical sliders also. Can't we adjust the event.pointer.position back to a position that makes sense to the position relative to the HSlider (basically just account for that rotation because using x's and y's).

This could be as easy as conditionals based on an option (although it is not elegant in this form yet).


if( options.isActuallyVerticalEvenThoughTheHInHSliderMeansHorizontal){
          clickXOffset = transform.inversePosition2( event.pointer.point ).y - thumb.y;
}
else{
          clickXOffset = transform.inversePosition2( event.pointer.point ).x - thumb.x;
}
samreid commented 7 years ago

Will we ever want to have a slider at an arbitrary angle? Perhaps we can do some trigonometry and get the correct answer for all angles?

zepumph commented 7 years ago

I wanted to make sure we were barking up the right tree, and it turns out we aren't. I think this is just a problem with HSlider in general, and not because of the transform.

To figure this out I jury rigged ESPB with this same image instead of the default slider. See here, the problem is the same. hsliderriaw hslideresbp

zepumph commented 7 years ago

This will be handed in phetsims/sun#288. Closing