phetsims / number-suite-common

"Number Suite Common" contains code for use by sims that are part of the Number Suite
GNU General Public License v3.0
0 stars 0 forks source link

Calculate margins for organization spots #68

Closed zepumph closed 1 year ago

zepumph commented 1 year ago

Promoting TODO to issue:

https://github.com/phetsims/number-suite-common/blob/4276977f00e397e5908d0c19e6a637deb438b341/js/common/model/CountingArea.ts#L368-L370

@chrisklus and I spent some time on this and found that the 0,0 position of countingObjects were being considered the 0 of the object position for x, but the top of the card for y. We will want to continue with this patch to figure out what is best:

```diff Subject: [PATCH] rename constants too, https://github.com/phetsims/number-suite-common/issues/40 --- Index: js/common/view/CountingAreaNode.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/view/CountingAreaNode.ts b/js/common/view/CountingAreaNode.ts --- a/js/common/view/CountingAreaNode.ts (revision 69ffef407921cbec1b52d706d79ab4dbd40e52fe) +++ b/js/common/view/CountingAreaNode.ts (date 1680563081138) @@ -226,7 +226,7 @@ * Add listener calls are duplicated from MakeATenExploreScreenView.onCountingObjectAdded */ public onCountingObjectAdded( countingObject: CountingObject ): void { - + console.log( countingObject ); const countingObjectNode = new CountingObjectNode( countingObject, this.countingAreaBoundsProperty, Index: js/common/model/CountingArea.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/common/model/CountingArea.ts b/js/common/model/CountingArea.ts --- a/js/common/model/CountingArea.ts (revision 69ffef407921cbec1b52d706d79ab4dbd40e52fe) +++ b/js/common/model/CountingArea.ts (date 1680562977072) @@ -52,7 +52,7 @@ private getCountingObjectOrigin: () => Vector2; private boundsProperty: TReadOnlyProperty; - private organizedObjectSpots: Vector2[]; + private organizedObjectSpots: Vector2[]; // Positions point to the top and left of the spot it should take up. // true when this.getCountingObjectOrigin() and this.boundsProperty have been set private initialized: boolean; @@ -362,13 +362,17 @@ const objectHeight = CountingCommonConstants.SINGLE_COUNTING_OBJECT_BOUNDS.height; const objectMargin = 3; - const numberOfColumns = 5; // rows + const numberOfColumns = 5; const numberOfRows = this.sumProperty.range.max / numberOfColumns; - //TODO https://github.com/phetsims/number-suite-common/issues/68 figure out why math isn't working for this - const xMargin = 88; // empirically determined to center group - const yMargin = CountingCommonConstants.COUNTING_AREA_MARGIN; + //TODO https://github.com/phetsims/number-suite-common/issues/68 figure out why math isn't working for this (this.boundsProperty.value.width-(numberOfColumns*(objectWidth+objectMargin)))/2=== 67?!?!?! + const xMargin = 0; + + // const xMargin = ( this.boundsProperty.value.width - ( numberOfColumns * ( objectWidth + objectMargin ) ) + objectWidth ) / 2; + // const xMargin = 88; // empirically determined to center group + const yMargin = 0; + console.log( this.boundsProperty.value ); const spots = []; for ( let i = 0; i < numberOfRows; i++ ) { @@ -379,6 +383,7 @@ ) ); } } + console.log( spots ); return spots; } @@ -399,22 +404,30 @@ this.breakApartCountingObjects(); // Copy the current countingObjects in the countingArea so we can mutate them. - let objectsToOrganize = this.getCountingObjectsIncludedInSum(); - const numberOfObjectsToOrganize = objectsToOrganize.length; + let countingObjectsToOrganize = this.getCountingObjectsIncludedInSum(); + const numberOfObjectsToOrganize = countingObjectsToOrganize.length; for ( let i = 0; i < numberOfObjectsToOrganize; i++ ) { - const destination = this.organizedObjectSpots[ i ]; + const spot = this.organizedObjectSpots[ i ]; // Sort the countingObjects by closest to the destination. - objectsToOrganize = _.sortBy( objectsToOrganize, object => { - return object.positionProperty.value.distance( destination ); + countingObjectsToOrganize = _.sortBy( countingObjectsToOrganize, countingObject => { + return countingObject.positionProperty.value.distance( spot ); } ); - const objectToOrganize = objectsToOrganize.shift(); + const countingObjectToOrganize = countingObjectsToOrganize.shift()!; - objectToOrganize && objectToOrganize.setDestination( destination, true, { + // the organization spot is in the top/left of the spot + // const destination = spot.minus( countingObjectToOrganize.localBounds.leftTop ); + // console.log( spot, countingObjectToOrganize.localBounds.leftTop ); + + // TODO + console.log( countingObjectToOrganize.positionProperty.value, spot ); + countingObjectToOrganize.setDestination( spot, true, { targetScale: NumberSuiteCommonConstants.COUNTING_OBJECT_SCALE } ); } + + assert && assert( countingObjectsToOrganize.length === 0, 'should have handled all countingObjects' ); } /** @@ -424,8 +437,8 @@ * @param groupAndLinkType */ public matchCountingObjectsToLinkedCountingArea( countingObjectSerializations: CountingObjectSerialization[], - linkStatusChangedEmitter: TEmitter<[ boolean ]>, areObjectsLinkedToOnes: boolean, - groupAndLinkType: GroupAndLinkType ): void { + linkStatusChangedEmitter: TEmitter<[ boolean ]>, areObjectsLinkedToOnes: boolean, + groupAndLinkType: GroupAndLinkType ): void { if ( areObjectsLinkedToOnes ) { this.linkToSerializedCountingObjects( countingObjectSerializations, linkStatusChangedEmitter, areObjectsLinkedToOnes ); ```
zepumph commented 1 year ago

Promoted its own issue from https://github.com/phetsims/number-suite-common/issues/29

chrisklus commented 1 year ago

@zepumph and I figured this out and committed above, closing!