Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Bead Alt-Input Behavior #46

Open
marlitas opened this issue Dec 10, 2024 · 0 comments
Open

Implement Bead Alt-Input Behavior #46

marlitas opened this issue Dec 10, 2024 · 0 comments
Assignees

Comments

@marlitas
Copy link
Contributor

The sim is close to having the alt-input behavior added in for beads.

Unfortunately I am running into reentrancy issues. I can avoid this with the SoundDragListener because the event provides information on the proposed new position through the pointerPoint. SoundKeyboardDragListener events naturally don't have this same information. The new position is calculated in KeyboardDragListener which directly updates the xPositionProperty. I am having a hard time navigating how to handle this without writing my own implementation of KeyboardDragListener, which I really don't want to do. @pixelzoom is going to take a look at the code with the below patch to see if he can make heads or tails of the reentrancy errors occurring for selectedBeadPositionProperty.

Subject: [PATCH] beads reentrancy
---
Index: js/common/view/BeadsOnWireNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BeadsOnWireNode.ts b/js/common/view/BeadsOnWireNode.ts
--- a/js/common/view/BeadsOnWireNode.ts	(revision 826cdf29b66956ecd28e5fdf777e82cb4902e3b8)
+++ b/js/common/view/BeadsOnWireNode.ts	(date 1733868917915)
@@ -164,8 +164,8 @@
               this.beadDragging = true;
             }
           },
-          onDrag: ( pointerPoint: Vector2, beadNode: BeadNode ) => {
-            this.handleBeadMove( pointerPoint, beadNode );
+          onDrag: ( pointerPoint: Vector2, currentCenterPoint: Vector2, beadNode: BeadNode ) => {
+            this.handleBeadMove( pointerPoint, currentCenterPoint, beadNode );
           },
           onEndDrag: () => {
             this.beadDragging = false;
@@ -175,13 +175,19 @@
       // TODO: Is it odd that we are using handleBeadMove here only for keyboard?
       //  If we want to put it here for mouse as well we need to create Properties about whether it is mouse grabbed and
       //  track which one is dragging.
-      //  Additional complications... this is reentrant... duh. I want to get the proposed value from keyboard before
-      //  deciding to set it there, but I don't see support for that in the KeyboardDragListener. Consult with CM
-      countingObject.beadXPositionProperty.link( x => {
-        if ( groupSelectModel.isGroupItemKeyboardGrabbedProperty.value && groupSelectModel.selectedGroupItemProperty.value === countingObject ) {
-          // this.handleBeadMove( new Vector2( this.modelViewTransform.modelToViewX( x ), 0 ), beadNode );
+      //  This patch is closer.
+      //  I have resolved the initial reentrancy complications, but I am still seeing reentrant issues in the DynamicProperty
+      //  selectedBeadPositionProperty. It seems to only happen in specific scenarios.
+      countingObject.beadXPositionProperty.link( ( x, oldX ) => {
+        const clearToMove = oldX && !Utils.equalsEpsilon( x, oldX, 0.001 );
+        if ( clearToMove && groupSelectModel.isGroupItemKeyboardGrabbedProperty.value && groupSelectModel.selectedGroupItemProperty.value === countingObject ) {
+          const newCenterX = this.handleBeadMove(
+            new Vector2( this.modelViewTransform.modelToViewX( x ), 0 ),
+            new Vector2( this.modelViewTransform.modelToViewX( oldX ), 0 ), beadNode );
+          beadNode.center = new Vector2( newCenterX, 0 );
         }
         else {
+          console.log( 'set center directly' );
           beadNode.center = new Vector2( modelViewTransform.modelToViewX( x ), 0 );
         }
       } );
@@ -229,15 +235,16 @@
   }
 
   /**
-   * Handle the movement of a bead and its neighbors when it is dragged.
-   * @param newPosition
+   * Handle the movement of a bead and its neighbors when it is dragged and return the grabbed Bead's new position
+   * for keyboard handling.
+   * @param proposedPosition
+   * @param oldPosition
    * @param grabbedBeadNode
    */
-  private handleBeadMove( newPosition: Vector2, grabbedBeadNode: BeadNode ): void {
-    const proposedParentPosition = grabbedBeadNode.globalToParentPoint( newPosition );
+  private handleBeadMove( proposedPosition: Vector2, oldPosition: Vector2, grabbedBeadNode: BeadNode ): number {
 
     // Determine whether we are dragging the bead to the right or left along the wire.
-    const draggingRight = Math.sign( newPosition.x - grabbedBeadNode.parentToGlobalPoint( grabbedBeadNode.bounds.center ).x ) > 0;
+    const draggingRight = Math.sign( proposedPosition.x - oldPosition.x ) > 0;
 
     // Reverse the sorted and active beads if we are dragging towards the left.
     const activeBeadNodes = this.getSortedBeadNodes();
@@ -258,7 +265,7 @@
         if ( !touchingPreviousBead ) {
           beadSpaceFound = true;
         }
-        const newPositionPastBead = draggingRight ? proposedParentPosition.x >= beadNode.centerX : proposedParentPosition.x <= beadNode.centerX;
+        const newPositionPastBead = draggingRight ? proposedPosition.x >= beadNode.centerX : proposedPosition.x <= beadNode.centerX;
 
         // We want to only return beads that are the same addend type as the grabbed bead, and are touching the
         // grabbed bead without space in between, OR the grabbed bead is proposing to move past a bead no matter
@@ -277,7 +284,7 @@
     const dragBoundsWithMovingBeads = this.beadDragBounds.withOffsets( minXOffset, 0, maxXOffset, 0 );
 
     // Constrain the new position to the drag bounds and set the grabbed bead's updated position.
-    const newCenterX = dragBoundsWithMovingBeads.closestPointTo( proposedParentPosition ).x;
+    const newCenterX = dragBoundsWithMovingBeads.closestPointTo( proposedPosition ).x;
     grabbedBeadNode.model.beadXPositionProperty.value = this.modelViewTransform.viewToModelX( newCenterX );
 
     // Since beadNodesToMove was created above by slicing the sortedBeadNodeArray at the grabbedBead, we can
@@ -290,7 +297,7 @@
 
         // Move the beads in the drag direction and base their positions on the grabbed bead.
         beadNode.model.beadXPositionProperty.value = this.modelViewTransform.viewToModelX(
-          grabbedBeadNode.centerX + i * ( draggingRight ? BEAD_WIDTH : -BEAD_WIDTH ) );
+          newCenterX + i * ( draggingRight ? BEAD_WIDTH : -BEAD_WIDTH ) );
       }
 
       /**
@@ -358,9 +365,10 @@
         }
       }
     } );
-
     assert && assert( this.leftAddendCountingObjectsProperty.value.length === this.model.leftAddendProperty.value, 'leftAddendObjects.length should match leftAddendNumberProperty' );
     assert && assert( this.rightAddendCountingObjectsProperty.value.length === this.model.rightAddendProperty.value, 'rightAddendObjects.length should match rightAddendNumberProperty' );
+
+    return newCenterX;
   }
 
   /**
Index: js/common/view/BeadNode.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/common/view/BeadNode.ts b/js/common/view/BeadNode.ts
--- a/js/common/view/BeadNode.ts	(revision 826cdf29b66956ecd28e5fdf777e82cb4902e3b8)
+++ b/js/common/view/BeadNode.ts	(date 1733868359949)
@@ -23,7 +23,7 @@
 
 type SelfOptions = {
   onStartDrag: ( beadNode: BeadNode ) => void;
-  onDrag: ( position: Vector2, beadNode: BeadNode ) => void;
+  onDrag: ( pointerPoint: Vector2, currentCenterPoint: Vector2, beadNode: BeadNode ) => void;
   onEndDrag: () => void;
 };
 
@@ -64,7 +64,7 @@
       // TODO: Ask CC if we want to use an offset here. See ShoppingItemDragListener for example. There's a weird scenario where it can make other beads jump
       // too sometimes I think we should handle the offset.
       start: () => options.onStartDrag( this ),
-      drag: event => options.onDrag( event.pointer.point, this ),
+      drag: event => options.onDrag( this.globalToParentPoint( event.pointer.point ), this.parentToGlobalPoint( this.center ), this ),
       end: options.onEndDrag,
       tandem: options.tandem.createTandem( 'dragListener' )
     } );
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants