From c39746dd28dd4565d5c468fceec79d3a8cdc7324 Mon Sep 17 00:00:00 2001 From: Sam Reid Date: Wed, 15 Jan 2025 08:14:54 -0700 Subject: [PATCH] Address ui interaction corner cases for tracks and control points, see https://github.com/phetsims/energy-skate-park/issues/385 --- js/common/model/ControlPoint.ts | 21 ++++++++++++++------ js/common/model/EnergySkateParkModel.ts | 4 ---- js/common/model/Track.ts | 26 +++++++------------------ js/common/view/ControlPointNode.ts | 15 ++++++++++---- js/common/view/TrackDragHandler.ts | 23 +++++++++++----------- 5 files changed, 44 insertions(+), 45 deletions(-) diff --git a/js/common/model/ControlPoint.ts b/js/common/model/ControlPoint.ts index 08b761ff..352c754c 100644 --- a/js/common/model/ControlPoint.ts +++ b/js/common/model/ControlPoint.ts @@ -18,8 +18,9 @@ import Vector2Property from '../../../../dot/js/Vector2Property.js'; import optionize from '../../../../phet-core/js/optionize.js'; import PhetioObject, { PhetioObjectOptions } from '../../../../tandem/js/PhetioObject.js'; import Tandem from '../../../../tandem/js/Tandem.js'; +import BooleanIO from '../../../../tandem/js/types/BooleanIO.js'; import IOType from '../../../../tandem/js/types/IOType.js'; -import NumberIO from '../../../../tandem/js/types/NumberIO.js'; +import NullableIO from '../../../../tandem/js/types/NullableIO.js'; import energySkatePark from '../../energySkatePark.js'; import EnergySkateParkModel from './EnergySkateParkModel.js'; @@ -117,15 +118,23 @@ export default class ControlPoint extends PhetioObject { valueType: ControlPoint, documentation: 'A control point that can manipulate the track.', toStateObject: controlPoint => { - return { x: controlPoint.positionProperty.value.x, y: controlPoint.positionProperty.value.y }; + return { + visible: controlPoint.visible, + interactive: controlPoint.interactive, + limitBounds: controlPoint.limitBounds + }; }, stateSchema: { - x: NumberIO, - y: NumberIO + visible: BooleanIO, + interactive: BooleanIO, + limitBounds: NullableIO( Bounds2.Bounds2IO ) }, stateObjectToCreateElementArguments: stateObject => { - assert && assert( typeof stateObject.x === 'number' ); - return [ stateObject.x, stateObject.y ]; + return [ 0, 0, { + visible: stateObject.visible, + interactive: stateObject.interactive, + limitBounds: stateObject.limitBounds + } ]; } } ); } diff --git a/js/common/model/EnergySkateParkModel.ts b/js/common/model/EnergySkateParkModel.ts index 99f0316d..5d734e27 100644 --- a/js/common/model/EnergySkateParkModel.ts +++ b/js/common/model/EnergySkateParkModel.ts @@ -1552,7 +1552,6 @@ export default class EnergySkateParkModel extends PhetioObject { this.controlPointGroup.disposeElement( controlPointToDelete ); const newTrack = this.trackGroup.createNextElement( points, Track.FULLY_INTERACTIVE_OPTIONS ); newTrack.physicalProperty.value = true; - newTrack.droppedProperty.value = true; // smooth out the new track, see #177 const smoothingPoint = controlPointIndex >= newTrack.controlPoints.length ? newTrack.controlPoints.length - 1 : controlPointIndex; @@ -1607,10 +1606,8 @@ export default class EnergySkateParkModel extends PhetioObject { const newTrack1 = this.trackGroup.createNextElement( points1, Track.FULLY_INTERACTIVE_OPTIONS ); newTrack1.physicalProperty.value = true; - newTrack1.droppedProperty.value = true; const newTrack2 = this.trackGroup.createNextElement( points2, Track.FULLY_INTERACTIVE_OPTIONS ); newTrack2.physicalProperty.value = true; - newTrack2.droppedProperty.value = true; track.removeEmitter.emit(); this.removeAndDisposeTrack( track ); @@ -1701,7 +1698,6 @@ export default class EnergySkateParkModel extends PhetioObject { const newTrack = this.trackGroup.createNextElement( points, Track.FULLY_INTERACTIVE_OPTIONS ); newTrack.physicalProperty.value = true; - newTrack.droppedProperty.value = true; a.disposeControlPoints(); a.removeEmitter.emit(); diff --git a/js/common/model/Track.ts b/js/common/model/Track.ts index ecc22145..d8c56d8e 100644 --- a/js/common/model/Track.ts +++ b/js/common/model/Track.ts @@ -104,7 +104,6 @@ export default class Track extends PhetioObject { // Flag to indicate whether the user has dragged the track out of the toolbox. If dragging from the toolbox, // then dragging translates the entire track instead of just a point. - public readonly droppedProperty: BooleanProperty; public readonly controlPoints: ControlPoint[]; public readonly controlPointDraggingProperty: TReadOnlyProperty; public readonly parametricPosition: Float64Array; @@ -185,20 +184,9 @@ export default class Track extends PhetioObject { phetioState: options.phetioState // Participate in state only if parent track is too } ); - this.leftThePanelProperty = new BooleanProperty( false, { - tandem: tandem.createTandem( 'leftThePanelProperty' ), - phetioState: options.phetioState // Participate in state only if parent track is too - } ); + this.leftThePanelProperty = new BooleanProperty( false ); - this.draggingProperty = new BooleanProperty( false, { - tandem: tandem.createTandem( 'draggingProperty' ), - phetioState: options.phetioState // Participate in state only if parent track is too - } ); - - this.droppedProperty = new BooleanProperty( false, { - tandem: tandem.createTandem( 'droppedProperty' ), - phetioState: options.phetioState // Participate in state only if parent track is too - } ); + this.draggingProperty = new BooleanProperty( false ); const trackChangedListener = () => { model.trackChangedEmitter.emit(); }; this.physicalProperty.link( trackChangedListener ); @@ -235,9 +223,9 @@ export default class Track extends PhetioObject { }; phetioStateSetEmitter.addListener( stateListener ); - // when available bounds change, make sure that control points are within - must be disposed + // when available bounds change, make sure that control points are within bounds - must be disposed const boundsListener = ( bounds: Bounds2 ) => { - if ( this.droppedProperty.get() ) { + if ( this.physicalProperty.value && this.splittable ) { this.containControlPointsInAvailableBounds( bounds ); } }; @@ -248,7 +236,6 @@ export default class Track extends PhetioObject { this.physicalProperty.dispose(); this.leftThePanelProperty.dispose(); this.draggingProperty.dispose(); - this.droppedProperty.dispose(); this.controlPointDraggingProperty.dispose(); this.model.availableModelBoundsProperty.unlink( boundsListener ); @@ -291,7 +278,6 @@ export default class Track extends PhetioObject { this.physicalProperty.reset(); this.leftThePanelProperty.reset(); this.draggingProperty.reset(); - this.droppedProperty.reset(); for ( let i = 0; i < this.controlPoints.length; i++ ) { this.controlPoints[ i ].reset(); } @@ -1054,7 +1040,9 @@ export default class Track extends PhetioObject { return [ controlPoints, { draggable: stateObject.draggable, - configurable: stateObject.configurable + configurable: stateObject.configurable, + splittable: stateObject.splittable, + attachable: stateObject.attachable } ]; }, stateSchema: { diff --git a/js/common/view/ControlPointNode.ts b/js/common/view/ControlPointNode.ts index ba461fea..5882d658 100644 --- a/js/common/view/ControlPointNode.ts +++ b/js/common/view/ControlPointNode.ts @@ -99,7 +99,7 @@ export default class ControlPointNode extends Circle { trackNode.moveToFront(); // If control point dragged out of the control panel, translate the entire track, see #130 - if ( !track.physicalProperty.value || ( !track.droppedProperty.value && track.draggable ) ) { + if ( !track.physicalProperty.value ) { // save drag handler so we can end the drag if we need to after forwarding if ( track.dragSource === null ) { @@ -120,12 +120,19 @@ export default class ControlPointNode extends Circle { if ( !model.containsTrack( track ) ) { return; } // If control point dragged out of the control panel, translate the entire track, see #130 - if ( !track.physicalProperty.value || ( !track.droppedProperty.value && track.draggable ) ) { + if ( !track.physicalProperty.value ) { // Only drag a track if nothing else was dragging the track (which caused a flicker), see #282 if ( track.dragSource === dragListener ) { - trackDragHandler && trackDragHandler.trackDragged( event ); + if ( trackDragHandler ) { + if ( trackDragHandler.startOffset === null ) { + trackDragHandler.handleDragStart( event, this.globalToParentPoint( event.pointer.point ) ); + } + + trackDragHandler.trackDragged( event, this.globalToParentPoint( event.pointer.point ) ); + } + } return; } @@ -196,7 +203,7 @@ export default class ControlPointNode extends Circle { model.userControlledPropertySet.trackControlledProperty.set( false ); // If control point dragged out of the control panel, translate the entire track, see #130 - if ( !track.physicalProperty.value || ( !track.droppedProperty.value && track.draggable ) ) { + if ( !track.physicalProperty.value ) { // Only drop a track if nothing else was dragging the track (which caused a flicker), see #282 if ( track.dragSource === dragListener ) { diff --git a/js/common/view/TrackDragHandler.ts b/js/common/view/TrackDragHandler.ts index 4f17ed7a..141e7023 100644 --- a/js/common/view/TrackDragHandler.ts +++ b/js/common/view/TrackDragHandler.ts @@ -23,7 +23,7 @@ export default class TrackDragHandler extends DragListener { private readonly model: EnergySkateParkModel; private readonly modelViewTransform: ModelViewTransform2; private readonly availableBoundsProperty: Property; - private readonly startOffset: Vector2 | null; + public readonly startOffset: Vector2 | null; /** * @param trackNode the track node that this listener will drag @@ -54,15 +54,15 @@ export default class TrackDragHandler extends DragListener { } /** - * Start of a drag interaction from an event. + * Start of a drag interaction from an event. TODO: clean up overrideParentPoint usages? See https://github.com/phetsims/energy-skate-park/issues/385 */ - public handleDragStart( event: SceneryEvent ): void { + public handleDragStart( event: SceneryEvent, overrideParentPoint: Vector2 | null = null ): void { // Move the track to the front when it starts dragging, see #296 // The track is in a layer of tracks (without other nodes) so moving it to the front will work perfectly this.trackNode.moveToFront(); - this.calculateStartOffset( event ); + this.calculateStartOffset( event, overrideParentPoint ); } /** @@ -81,7 +81,7 @@ export default class TrackDragHandler extends DragListener { this.trackDragEnded( event ); } - public trackDragged( event: SceneryEvent ): void { + public trackDragged( event: SceneryEvent, overrideParentPoint: Vector2 | null = null ): void { let snapTargetChanged = false; const model = this.model; const track = this.track; @@ -91,8 +91,9 @@ export default class TrackDragHandler extends DragListener { track.draggingProperty.value = true; + const myPt = overrideParentPoint || this.globalToParentPoint( event.pointer.point ); // @ts-expect-error - const parentPoint = this.globalToParentPoint( event.pointer.point ).minus( this.startOffset ); + const parentPoint = myPt.minus( this.startOffset ); const position = this.modelViewTransform.viewToModelPosition( parentPoint ); // If the user moved it out of the toolbox above y=0, then make it physically interactive @@ -214,10 +215,6 @@ export default class TrackDragHandler extends DragListener { const track = this.track; const model = this.model; - // If dropped in the play area, signify that it has been dropped--this will make it so that dragging the control points - // reshapes the track instead of translating it - track.droppedProperty.value = true; - // Check whether the model contains a track so that input listeners for detached elements can't create bugs, see #230 if ( !model.containsTrack( track ) ) { return; } @@ -248,11 +245,13 @@ export default class TrackDragHandler extends DragListener { /** * Determine the offset point at the start of a drag so that the track translates with the mouse without jumping. */ - private calculateStartOffset( event: SceneryEvent ): void { + private calculateStartOffset( event: SceneryEvent, overrideParentPoint: Vector2 | null = null ): void { const startingPosition = this.modelViewTransform.modelToViewPosition( this.track.position ); + const pt = overrideParentPoint || event.currentTarget!.globalToParentPoint( event.pointer.point ); + // @ts-expect-error - this.startOffset = event.currentTarget.globalToParentPoint( event.pointer.point ).minus( startingPosition ); + this.startOffset = pt.minus( startingPosition ); } }