phetsims / projectile-data-lab

"Projectile Data Lab" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Interval tool mouse gets disassociated from the center #280

Closed samreid closed 3 months ago

samreid commented 3 months ago

From #267, @matthew-blackman and I observed the mouse gets away from the center.

We tried this patch to help:

```diff Subject: [PATCH] Remove drag bounds for center dragging and update the edge management logic, see https://github.com/phetsims/projectile-data-lab/issues/274 --- Index: js/measures/view/IntervalToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/IntervalToolNode.ts b/js/measures/view/IntervalToolNode.ts --- a/js/measures/view/IntervalToolNode.ts (revision bd7e2a3603f4538aa96c42303d7d9ae975644152) +++ b/js/measures/view/IntervalToolNode.ts (date 1712080299864) @@ -22,7 +22,7 @@ import Utils from '../../../../dot/js/Utils.js'; import PatternStringProperty from '../../../../axon/js/PatternStringProperty.js'; import ProjectileDataLabStrings from '../../ProjectileDataLabStrings.js'; -import DerivedProperty from '../../../../axon/js/DerivedProperty.js'; +import DerivedProperty, { DerivedProperty1 } from '../../../../axon/js/DerivedProperty.js'; import PDLColors from '../../common/PDLColors.js'; import Property from '../../../../axon/js/Property.js'; import SoundClip from '../../../../tambo/js/sound-generators/SoundClip.js'; @@ -394,13 +394,28 @@ drag: moveToFront( edge2Sphere ) }, edge2DragListenerOptions, dragListenerOptions ) ) ); + // This is where the UI wants to put the center. + const desiredCenterLocationProperty = new Property( new Vector2( 50, 0 ) ); + + desiredCenterLocationProperty.lazyLink( ( value: Vector2, oldValue: Vector2 ) => { + intervalTool.centerProperty.value = value.x; + } ); + desiredCenterLocationProperty.debug( 'desired center: ' ); + + const centerDragBoundsProperty = new DerivedProperty( [ intervalTool.edge1Property, intervalTool.edge2Property ], ( edge1, edge2 ) => { + const separation = Math.abs( edge2 - edge1 ); + console.log( 'center drag bounds changed: ' + separation ); + return new Bounds2( 0 + separation / 2, -1000, PDLConstants.MAX_FIELD_DISTANCE - separation / 2, 1000 ); + } ); + readoutVBox.addInputListener( new DragListener( combineOptions>( { applyOffset: true, useParentOffset: true, - positionProperty: createDynamicAdapterProperty( intervalTool.centerProperty, true ), + positionProperty: desiredCenterLocationProperty, tandem: providedOptions.tandem.createTandem( 'centerDragListener' ) }, centerDragListenerOptions, { - useInputListenerCursor: true + useInputListenerCursor: true, + dragBoundsProperty: centerDragBoundsProperty } ) ) ); // Play a ratcheting sound as either edge is dragged. The sound is played when passing thresholds on the field, Index: js/measures/model/IntervalTool.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/model/IntervalTool.ts b/js/measures/model/IntervalTool.ts --- a/js/measures/model/IntervalTool.ts (revision bd7e2a3603f4538aa96c42303d7d9ae975644152) +++ b/js/measures/model/IntervalTool.ts (date 1712080189471) @@ -75,7 +75,9 @@ // This Property is for input only, for use in the drag listener. Do not try to read the value, instead average the edges. // This works because DragListener only sets value and does not read values for this property (do not use _useParentOffset // in the node that drags based on this Property.) - this.centerProperty = new NumberProperty( ( DEFAULT_EDGE_1 + DEFAULT_EDGE_2 ) / 2 ); + this.centerProperty = new NumberProperty( ( DEFAULT_EDGE_1 + DEFAULT_EDGE_2 ) / 2, { + reentrant: true + } ); this.centerProperty.lazyLink( ( center, oldCenter ) => { @@ -105,8 +107,18 @@ edge1 += delta; edge2 += delta; + // Treat atomically + this.edge1Property.setDeferred( true ); + this.edge2Property.setDeferred( true ); + this.edge1Property.value = edge1; this.edge2Property.value = edge2; + + const notifier1 = this.edge1Property.setDeferred( false ); + const notifier2 = this.edge2Property.setDeferred( false ); + + notifier1 && notifier1(); + notifier2 && notifier2(); } ); this.dataFractionProperty = new NumberProperty( 0, { @@ -115,6 +127,14 @@ phetioDocumentation: 'The fraction of data within the interval tool', phetioReadOnly: true } ); + + // this.edge1Property.link( edge1 => { + // this.centerProperty.value = ( edge1 + this.edge2Property.value ) / 2; + // } ); + // + // this.edge2Property.link( edge2 => { + // this.centerProperty.value = ( this.edge1Property.value + edge2 ) / 2; + // } ); } public reset(): void {
samreid commented 3 months ago

This one is almost working:

```diff Subject: [PATCH] Remove drag bounds for center dragging and update the edge management logic, see https://github.com/phetsims/projectile-data-lab/issues/274 --- Index: js/measures/view/IntervalToolNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/view/IntervalToolNode.ts b/js/measures/view/IntervalToolNode.ts --- a/js/measures/view/IntervalToolNode.ts (revision bd7e2a3603f4538aa96c42303d7d9ae975644152) +++ b/js/measures/view/IntervalToolNode.ts (date 1712083857651) @@ -40,6 +40,7 @@ import DynamicProperty from '../../../../axon/js/DynamicProperty.js'; import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import isResettingAllProperty from '../../common/model/isResettingAllProperty.js'; +import Multilink from '../../../../axon/js/Multilink.js'; const edgeFilter = new BiquadFilterNode( phetAudioContext, { type: 'lowpass', @@ -128,7 +129,7 @@ // This is a downstream Property (only updated in the update function) that is used for sonification and the center position label.. // IntervalTool.centerXProperty may go out of range, so we use this Property to clamp the value. - const boundedCenterXProperty = new NumberProperty( intervalTool.centerProperty.value ); + const boundedCenterXProperty = new NumberProperty( 50 ); class DraggableShadedSphereNode extends AccessibleSlider( Node, 0 ) { public constructor( options: AccessibleSliderOptions ) { @@ -273,7 +274,7 @@ } const readoutVBox = new ReadoutVBox( { - valueProperty: intervalTool.centerProperty, + valueProperty: new Property( 5 ), enabledRangeProperty: new Property( new Range( 0, PDLConstants.MAX_FIELD_DISTANCE ) ) } ); this.addChild( readoutVBox ); @@ -394,13 +395,53 @@ drag: moveToFront( edge2Sphere ) }, edge2DragListenerOptions, dragListenerOptions ) ) ); + // This is where the UI wants to put the center. + const desiredCenterLocationProperty = new Property( new Vector2( 50, 0 ), { + valueComparisonStrategy: 'equalsFunction', + reentrant: true + } ); + + desiredCenterLocationProperty.lazyLink( ( value: Vector2, oldValue: Vector2 ) => { + if ( this.isCenterDraggingProperty.value ) { + intervalTool.edge1Property.value += value.x - oldValue.x; + intervalTool.edge2Property.value += value.x - oldValue.x; + } + } ); + + this.isEdge1DraggingProperty.link( dragging => { + if ( !dragging ) { + desiredCenterLocationProperty.value = new Vector2( ( intervalTool.edge1Property.value + intervalTool.edge2Property.value ) / 2, 0 ); + } + } ); + + this.isEdge2DraggingProperty.link( dragging => { + if ( !dragging ) { + desiredCenterLocationProperty.value = new Vector2( ( intervalTool.edge1Property.value + intervalTool.edge2Property.value ) / 2, 0 ); + } + } ); + + desiredCenterLocationProperty.debug( 'desired center: ' ); + + const separation = Math.abs( intervalTool.edge1Property.value - intervalTool.edge2Property.value ); + const centerDragBoundsProperty = new Property( new Bounds2( 0 + separation / 2, -1000, PDLConstants.MAX_FIELD_DISTANCE - separation / 2, 1000 ) ); + + Multilink.multilink( [ intervalTool.edge1Property, intervalTool.edge2Property, this.isCenterDraggingProperty, this.isEdge1DraggingProperty, this.isEdge2DraggingProperty ], () => { + if ( ( this.isEdge1DraggingProperty.value || this.isEdge2DraggingProperty.value ) && !this.isCenterDraggingProperty.value ) { + desiredCenterLocationProperty.value = new Vector2( ( intervalTool.edge1Property.value + intervalTool.edge2Property.value ) / 2, 0 ); + + const separation = Math.abs( intervalTool.edge1Property.value - intervalTool.edge2Property.value ); + centerDragBoundsProperty.value = new Bounds2( 0 + separation / 2, -1000, PDLConstants.MAX_FIELD_DISTANCE - separation / 2, 1000 ); + } + } ); + readoutVBox.addInputListener( new DragListener( combineOptions>( { applyOffset: true, useParentOffset: true, - positionProperty: createDynamicAdapterProperty( intervalTool.centerProperty, true ), + positionProperty: desiredCenterLocationProperty, tandem: providedOptions.tandem.createTandem( 'centerDragListener' ) }, centerDragListenerOptions, { - useInputListenerCursor: true + useInputListenerCursor: true, + dragBoundsProperty: centerDragBoundsProperty } ) ) ); // Play a ratcheting sound as either edge is dragged. The sound is played when passing thresholds on the field, Index: js/measures/model/IntervalTool.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/measures/model/IntervalTool.ts b/js/measures/model/IntervalTool.ts --- a/js/measures/model/IntervalTool.ts (revision bd7e2a3603f4538aa96c42303d7d9ae975644152) +++ b/js/measures/model/IntervalTool.ts (date 1712083857642) @@ -36,11 +36,6 @@ public readonly edge1Property: Property; public readonly edge2Property: Property; - // This Property can be used to read and write the center of the interval tool. It is necessary for wiring up to the - // DragListener that translates the entire IntervalTool. This value may go out of the bounds of the field while dragging. - // This is preferable to updating and maintaining an allowed range that changes based on the width of the tool. - public readonly centerProperty: Property; - public constructor( providedOptions: IntervalToolOptions ) { const options = optionize()( { @@ -57,7 +52,8 @@ units: 'm', rangePropertyOptions: { tandem: Tandem.OPT_OUT - } + }, + reentrant: true } ); this.edge2Property = new NumberProperty( DEFAULT_EDGE_2, { @@ -66,47 +62,8 @@ units: 'm', rangePropertyOptions: { tandem: Tandem.OPT_OUT - } - } ); - - // This one is not PhET-iO instrumented, to avoid bounds and circularity issues. Clients should use edge1Property and edge2Property. - // This is a transient Property used to get drag events when dragging the center. It just passes through to the - // edges. The view is centered by averaging the edges. - // This Property is for input only, for use in the drag listener. Do not try to read the value, instead average the edges. - // This works because DragListener only sets value and does not read values for this property (do not use _useParentOffset - // in the node that drags based on this Property.) - this.centerProperty = new NumberProperty( ( DEFAULT_EDGE_1 + DEFAULT_EDGE_2 ) / 2 ); - - this.centerProperty.lazyLink( ( center, oldCenter ) => { - - let delta = center - oldCenter; - - let edge1 = this.edge1Property.value; - let edge2 = this.edge2Property.value; - - if ( delta > 0 ) { - - const max = Math.max( edge1, edge2 ); - if ( max + delta > PDLConstants.MAX_FIELD_DISTANCE ) { - delta = PDLConstants.MAX_FIELD_DISTANCE - max; - } - } - else if ( delta < 0 ) { - - const min = Math.min( edge1, edge2 ); - if ( min + delta < 0 ) { - delta = -min; - } - } - else { - // nothing to do - } - - edge1 += delta; - edge2 += delta; - - this.edge1Property.value = edge1; - this.edge2Property.value = edge2; + }, + reentrant: true } ); this.dataFractionProperty = new NumberProperty( 0, { @@ -120,7 +77,6 @@ public reset(): void { this.edge1Property.reset(); this.edge2Property.reset(); - this.centerProperty.reset(); } }
samreid commented 3 months ago

I committed the work in https://github.com/phetsims/projectile-data-lab/commit/baec4f886602c4b4a892a2a01dd0f300797855cf

Remaning/to discuss:

samreid commented 3 months ago

Dragging center and edge with multitouch on iphone did not cause problems or crash it, so I think this issue can be closed.