Skip to content

Commit

Permalink
Ignore bounds with keyboard home and end, see: #106
Browse files Browse the repository at this point in the history
  • Loading branch information
@marlitas
marlitas committed Feb 26, 2025
1 parent 9f1b4d3 commit 771f3d6
Show file tree
Hide file tree
Showing 3 changed files with 70 additions and 33 deletions.
20 changes: 14 additions & 6 deletions js/common/model/BeadManager.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
* @author Marla Schulz (PhET Interactive Simulations)
*
*/
import NumberPairsModel, { BeadXPositionsTypes } from './NumberPairsModel.js';
import { BeadXPositionsTypes } from './NumberPairsModel.js';
import NumberPairsConstants from '../NumberPairsConstants.js';
import Range from '../../../../dot/js/Range.js';
import CountingObject, { AddendType } from './CountingObject.js';
Expand All @@ -24,7 +24,7 @@ export default class BeadManager {
public static readonly BEAD_WIDTH = 21.5;
public static readonly BEAD_HEIGHT = 80;
public static readonly LEFTMOST_BEAD_X = 1;
public static readonly RIGHTMOST_BEAD_X = Math.floor( NumberPairsConstants.COUNTING_AREA_BOUNDS.width / BeadManager.BEAD_WIDTH );
public static readonly RIGHTMOST_BEAD_X = Math.floor( NumberPairsConstants.COUNTING_AREA_BOUNDS.width / BeadManager.BEAD_WIDTH ) - 1;

// Although this model view transform is essentially used as a linear function, it is needed as the transform
// for the keyboard drag listener.
Expand Down Expand Up @@ -57,7 +57,7 @@ export default class BeadManager {
public updateBeadPositions( leftAddend: number ): void {
const leftAddendBeads = this.leftAddendCountingObjectsProperty.value;
const rightAddendBeads = this.rightAddendCountingObjectsProperty.value;
const separatorXPosition = NumberPairsModel.calculateBeadSeparatorXPosition( leftAddend );
const separatorXPosition = BeadManager.calculateBeadSeparatorXPosition( leftAddend );
const beadDistanceFromSeparator = NumberPairsConstants.BEAD_DISTANCE_FROM_SEPARATOR;

// Sort the bead x positions by addend type. If the bead is inactive it means it was removed, but we still
Expand Down Expand Up @@ -190,11 +190,11 @@ export default class BeadManager {
}

/**
* @param xPositions
* @param xPositions - in model coordinates
* @param direction - positive when we want to shift to the right, negative when we want to shift to the left.
* @param startingValue
*/
private shiftXPositions( xPositions: number[], direction: number, startingValue: number ): number[] {
public shiftXPositions( xPositions: number[], direction: number, startingValue: number ): number[] {
direction = Math.sign( direction );
const shiftedPositions: number[] = [];
xPositions.reduce( ( previousX, currentX ) => {
Expand Down Expand Up @@ -257,14 +257,22 @@ export default class BeadManager {
*/
public static getDefaultBeadPositions( leftAddendValue: number, rightAddendValue: number ): BeadXPositionsTypes {
const distanceFromSeparator = NumberPairsConstants.BEAD_DISTANCE_FROM_SEPARATOR;
const beadSeparatorXPosition = NumberPairsModel.calculateBeadSeparatorXPosition( leftAddendValue );
const beadSeparatorXPosition = BeadManager.calculateBeadSeparatorXPosition( leftAddendValue );

return {
leftAddendXPositions: _.times( leftAddendValue, i => beadSeparatorXPosition - i - distanceFromSeparator ),
rightAddendXPositions: _.times( rightAddendValue, i => i + beadSeparatorXPosition + distanceFromSeparator )
};
}

public static calculateBeadSeparatorXPosition( leftAddendValue: number ): number {

// empirically determined. This starting position is closely intertwined with the
// both the width of the bead, and the denominator in the calculation below.
const startingPosition = 15;
return leftAddendValue / 2.2 + startingPosition;
}

public reset(): void {
// Do I need to reset anything?
}
Expand Down
14 changes: 3 additions & 11 deletions js/common/model/NumberPairsModel.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,8 @@ export default class NumberPairsModel implements TModel {
if ( this.representationTypeProperty.validValues?.includes( RepresentationType.BEADS ) ) {

// Bead positions should be a mirrored translation across the separator.
const previousSeparatorPosition = NumberPairsModel.calculateBeadSeparatorXPosition( this.rightAddendProperty.value );
const updatedSeparatorPosition = NumberPairsModel.calculateBeadSeparatorXPosition( this.leftAddendProperty.value );
const previousSeparatorPosition = BeadManager.calculateBeadSeparatorXPosition( this.rightAddendProperty.value );
const updatedSeparatorPosition = BeadManager.calculateBeadSeparatorXPosition( this.leftAddendProperty.value );
const distanceBetweenSeparators = updatedSeparatorPosition - previousSeparatorPosition;
const newLeftBeadXPositions = rightBeadXPositions.map( x => Math.max( x - Math.abs( x - previousSeparatorPosition ) * 2 + distanceBetweenSeparators, BeadManager.LEFTMOST_BEAD_X ) );
const newRightBeadXPositions = leftBeadXPositions.map( x => Math.min( x + Math.abs( x - previousSeparatorPosition ) * 2 + distanceBetweenSeparators, BeadManager.RIGHTMOST_BEAD_X ) );
Expand Down Expand Up @@ -533,7 +533,7 @@ export default class NumberPairsModel implements TModel {
const distanceFromSeparator = 1.5;

// Beads should be lined up on the wire in groups of 5, with the remainder closest to the bead separator.
const beadSeparatorXPosition = NumberPairsModel.calculateBeadSeparatorXPosition( leftAddend );
const beadSeparatorXPosition = BeadManager.calculateBeadSeparatorXPosition( leftAddend );
const leftAddendRemainder = leftAddendBeads.length % 5;
const leftXPositions = leftAddendBeads.map( ( bead, i ) => {

Expand Down Expand Up @@ -711,14 +711,6 @@ export default class NumberPairsModel implements TModel {
assert && assert( this.leftAddendCountingObjectsProperty.value.length === this.leftAddendProperty.value, 'Addend array length and value should match' );
assert && assert( this.rightAddendCountingObjectsProperty.value.length === this.rightAddendProperty.value, 'Addend array length and value should match' );
}

public static calculateBeadSeparatorXPosition( leftAddendValue: number ): number {

// empirically determined. This starting position is closely intertwined with the
// both the width of the bead, and the denominator in the calculation below.
const startingPosition = 15;
return leftAddendValue / 2.2 + startingPosition;
}
}

numberPairs.register( 'NumberPairsModel', NumberPairsModel );
69 changes: 53 additions & 16 deletions js/common/view/BeadsOnWireNode.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import { equalsEpsilon } from '../../../../dot/js/util/equalsEpsilon.js';
import { clamp } from '../../../../dot/js/util/clamp.js';

const BEAD_WIDTH = BeadManager.BEAD_WIDTH;
const SEPARATOR_BUFFER = 1.5 * BEAD_WIDTH;
const WIRE_HEIGHT = 4;
type SelfOptions = {
sceneRange: Range;
Expand All @@ -63,6 +64,10 @@ export default class BeadsOnWireNode extends Node {
private readonly rightAddendCountingObjectsProperty: TReadOnlyProperty<ObservableArray<CountingObject>>;

private beadDragging = false;

// This flag is used during `home` and `end` keyboard behavior for the beads. We ignore bounds initially and then
// shift positions accordingly once all the work is done.
private ignoreBeadBounds = false;
private readonly keyboardProposedBeadPositionProperty: Vector2Property;

public constructor(
Expand Down Expand Up @@ -164,10 +169,21 @@ export default class BeadsOnWireNode extends Node {
const currentBeadXPosition = groupItem.beadXPositionProperty.value;

if ( keysPressed.includes( 'home' ) && BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX( currentBeadXPosition ) > separatorXPosition ) {
this.handleBeadMove( this.localToGlobalPoint( new Vector2( separatorXPosition - BEAD_WIDTH, 0 ) ), this.beadModelToNodeMap.get( groupItem )! );
this.ignoreBeadBounds = true;
this.handleBeadMove( this.localToGlobalPoint( new Vector2( separatorXPosition - SEPARATOR_BUFFER, 0 ) ), this.beadModelToNodeMap.get( groupItem )! );
this.ignoreBeadBounds = false;

const leftAddendSortedBeads = this.getSortedBeadNodes().filter( beadNode => beadNode.countingObject.addendTypeProperty.value === AddendType.LEFT );
this.handleBeadsOutsideOfBounds( leftAddendSortedBeads, true );

}
else if ( keysPressed.includes( 'end' ) && BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX( currentBeadXPosition ) < separatorXPosition ) {
this.handleBeadMove( this.localToGlobalPoint( new Vector2( separatorXPosition + BEAD_WIDTH, 0 ) ), this.beadModelToNodeMap.get( groupItem )! );
this.ignoreBeadBounds = true;
this.handleBeadMove( this.localToGlobalPoint( new Vector2( separatorXPosition + SEPARATOR_BUFFER, 0 ) ), this.beadModelToNodeMap.get( groupItem )! );
this.ignoreBeadBounds = false;

const rightAddendSortedBeads = this.getSortedBeadNodes().filter( beadNode => beadNode.countingObject.addendTypeProperty.value === AddendType.RIGHT ).reverse();
this.handleBeadsOutsideOfBounds( rightAddendSortedBeads, true );
}
},
tandem: options.tandem.createTandem( 'groupSelectView' )
Expand Down Expand Up @@ -222,7 +238,7 @@ export default class BeadsOnWireNode extends Node {
// If we are not dragging a bead was added or removed from the wire.
// We also want to make sure that our values are in sync during state or scene changes.
if ( !this.beadDragging && leftAddend === leftAddendLength && rightAddend === rightAddendLength ) {
this.beadSeparatorCenterXProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX( NumberPairsModel.calculateBeadSeparatorXPosition( leftAddend ) );
this.beadSeparatorCenterXProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX( BeadManager.calculateBeadSeparatorXPosition( leftAddend ) );

// We are only adding or removing beads in the sum screen.
// Reset may also give the impression that we are adding or removing beads, but we want to position the beads
Expand Down Expand Up @@ -250,13 +266,31 @@ export default class BeadsOnWireNode extends Node {
}
}

/**
* If a bead is outside the bounds of the drag area, shift all the beads of that addend type over to accommodate.
* @param movingRight
* @param beadNodes
*/
private handleBeadsOutsideOfBounds( beadNodes: BeadNode[], movingRight: boolean ): void {
const direction = movingRight ? 1 : -1;
const extremePosition = movingRight ? BeadManager.RIGHTMOST_BEAD_X : BeadManager.LEFTMOST_BEAD_X;

if ( _.some( beadNodes, beadNode => !this.beadDragBounds.containsPoint( beadNode.center ) ) ) {
const beadXPositions = beadNodes.map( beadNode => beadNode.countingObject.beadXPositionProperty.value );
const shiftedXPositions = this.model.beadManager.shiftXPositions( beadXPositions, direction, extremePosition );
beadNodes.forEach( ( beadNode, i ) => {
beadNode.countingObject.beadXPositionProperty.value = shiftedXPositions[ i ];
} );
}
}

/**
* Handle the movement of a bead and its neighbors when it is dragged.
* @param newPosition - the proposed new position, in the global view coordinate frame.
* @param grabbedBeadNode
*/
private handleBeadMove( newPosition: Vector2, grabbedBeadNode: BeadNode ): void {
const proposedParentPosition = grabbedBeadNode.globalToParentPoint( newPosition );
let proposedParentPosition = grabbedBeadNode.globalToParentPoint( newPosition );

// 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;
Expand All @@ -275,13 +309,16 @@ export default class BeadsOnWireNode extends Node {
*
* We should only adjust the bounds in the direction the bead is being dragged.
*/
const minXOffset = draggingRight ? 0 : -( beadNodesToMove.length - 1 ) * BEAD_WIDTH;
const maxXOffset = draggingRight ? -( beadNodesToMove.length - 1 ) * BEAD_WIDTH : 0;
const dragBoundsWithMovingBeads = this.beadDragBounds.withOffsets( minXOffset, 0, maxXOffset, 0 );
if ( !this.ignoreBeadBounds ) {
const minXOffset = draggingRight ? 0 : -( beadNodesToMove.length - 1 ) * BEAD_WIDTH;
const maxXOffset = draggingRight ? -( beadNodesToMove.length - 1 ) * BEAD_WIDTH : 0;
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.
proposedParentPosition = dragBoundsWithMovingBeads.closestPointTo( proposedParentPosition );
}

// Constrain the new position to the drag bounds and set the grabbed bead's updated position.
const newCenterX = dragBoundsWithMovingBeads.closestPointTo( proposedParentPosition ).x;
grabbedBeadNode.countingObject.beadXPositionProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.viewToModelX( newCenterX );
grabbedBeadNode.countingObject.beadXPositionProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.viewToModelX( proposedParentPosition.x );

// Since beadNodesToMove was created above by slicing the sortedBeadNodeArray at the grabbedBead, we can
// be confident that the first beadNode in the beadNodesToMove array is the grabbedBeadNode, and rely
Expand Down Expand Up @@ -311,7 +348,7 @@ export default class BeadsOnWireNode extends Node {

// Since a bead is moving to the right, the separator should adjust one position to the left.
this.beadSeparatorCenterXProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX(
NumberPairsModel.calculateBeadSeparatorXPosition( this.model.leftAddendProperty.value - 1 ) );
BeadManager.calculateBeadSeparatorXPosition( this.model.leftAddendProperty.value - 1 ) );

// Add the bead to the right addend first to avoid duplicate work being done when the left addend value is
// updated in the ObservableArray.lengthProperty listener.
Expand All @@ -321,7 +358,7 @@ export default class BeadsOnWireNode extends Node {
beadNode.countingObject.traverseInactiveObjects = true;

// As the bead moves past the separator recalculate if other beads now need to move to accommodate.
const xPosition = Math.max( newCenterX, this.beadSeparatorCenterXProperty.value + BEAD_WIDTH * 1.5 );
const xPosition = Math.max( beadNode.centerX, this.beadSeparatorCenterXProperty.value + SEPARATOR_BUFFER );
this.getBeadsToMove( beadNode, xPosition, true, sortedBeadNodes ).forEach( ( beadNode, i ) => {
beadNode.countingObject.beadXPositionProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.viewToModelX( xPosition + i * BEAD_WIDTH );
} );
Expand All @@ -335,7 +372,7 @@ export default class BeadsOnWireNode extends Node {

// Since a bead is moving to the left, the separator should adjust one position to the right.
this.beadSeparatorCenterXProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewX(
NumberPairsModel.calculateBeadSeparatorXPosition( this.model.leftAddendProperty.value + 1 ) );
BeadManager.calculateBeadSeparatorXPosition( this.model.leftAddendProperty.value + 1 ) );

// Remove the bead from the right addend first to avoid duplicate work being done when the left addend value is
// updated in the ObservableArray.lengthProperty listener.
Expand All @@ -345,7 +382,7 @@ export default class BeadsOnWireNode extends Node {
beadNode.countingObject.traverseInactiveObjects = true;

// As the bead moves past the separator recalculate if other beads now need to move to accommodate.
const xPosition = Math.min( newCenterX, this.beadSeparatorCenterXProperty.value - BEAD_WIDTH * 1.5 );
const xPosition = Math.min( beadNode.centerX, this.beadSeparatorCenterXProperty.value - SEPARATOR_BUFFER );
this.getBeadsToMove( beadNode, xPosition, false, sortedBeadNodes ).forEach( ( beadNode, i ) => {
beadNode.countingObject.beadXPositionProperty.value = BeadManager.BEAD_MODEL_VIEW_TRANSFORM.viewToModelX( xPosition - i * BEAD_WIDTH );
} );
Expand Down Expand Up @@ -396,8 +433,8 @@ export default class BeadsOnWireNode extends Node {
// If the bead is moving to the right, check if the proposed position plus the number of beads to move will go
// past the bead that is being considered. If it does, add the bead to the list of beads to move.
const newPositionPastBead = movingRight ?
proposedXPosition + proposedBeadsToMove.length * BEAD_WIDTH >= beadNode.centerX :
proposedXPosition - proposedBeadsToMove.length * BEAD_WIDTH <= beadNode.centerX;
proposedXPosition + BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewDeltaX( proposedBeadsToMove.length ) >= beadNode.centerX :
proposedXPosition - BeadManager.BEAD_MODEL_VIEW_TRANSFORM.modelToViewDeltaX( proposedBeadsToMove.length ) <= 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 any moving bead is proposing to move past another bead no matter
Expand Down

0 comments on commit 771f3d6

Please sign in to comment.