Refactor model to rely on sumProperty of play areas

[chrisklus]

Number Play has always duplicated where the "current number", or sum, is being tracked. There is a sumProperty internal to each play area (in counting-common), and Number Play also creates separate number Properties that represent the sum on the number play side of things. So for screens 1 and 2, there are 3 Properties trying to get along - Number Play has one number Property for the whole screen's model, and then each play area has a sumProperty. On the compare screen, each "side" has two number Properties - one from the play area, and one that Number Play creates in the model.

I think the reason I originally did this was because the sumProperty in the play areas had too much noise and I was afraid to make changes to the make-a-ten code (now counting-common). Meaning, for example, when combining two paper numbers in a play area, or pulling some apart, etc - for representations in Number Play, the sum should always stay the same during those kinds of interactions that don't ultimately result in a change of the sum. So I added additional number properties that had the appearance of being more stable. But this was a undesirable and buggy way to do it, so now I would like to eliminate all number-play specific sum Properties and just rely on the sumProperties in the play area. There are a couple main changes that need to happen to support this:

• [x] visit every case in counting-common that that is modifying sumProperty but not actually changing its value. Set the sumProperty as deferred during these modifications so the sum stays stable at the number it should be.
• [x] create a DerivedProperty for screens 1 and 2 (places that have 1 sum, but multiple play areas) so that number readouts etc can still have a single Property to watch, but it will be entirely dependent on the sums of the play areas. This will also be the place to keep the two play areas in sync, so they can follow each other if a user changes the sum in either one.
• [ ] added after commits below: fix errors that come up during fuzzing

[chrisklus]

@marlitas and I made good progress on this yesterday and then I was able to get to a commit point today. This was a very extensive change and eliminated a lot of bad code and at least 6 relevant TODOs.

There are going to be fuzz errors on CT because I think I am misunderstanding how not-synchronous dragging out new paper numbers can be when fuzzing, but I'm not sure. Anyway, there are still reentry issues and a couple other things related to keeping the play areas in sync on screens 1 and 2, so I'll reach out to @jonathanolson next week to get help. But the way things were, fuzzing was easily able to get the play areas out of sync, so this is definitely still an improvement since I think I have it pinned down to one place it's happening.

[chrisklus]

I worked on this more with @jonathanolson last week and we suspected that deferring the sumProperty at various times was perhaps causing a bug. So he recommended refactoring again to not calculate the sum every time the number of paper numbers changes, but instead just when ones are added or removed from the play area.

I completed that refactor and have all normal use working but fuzzing is still showing similar errors (reentry errors and "Assertion failed: A play area should not need to create more than one counting object at a time to match the opposite play area"). I can't figure out how to reproduce without fuzzing yet but will keep investigating.

[chrisklus]

@jonathanolson and I made good progress on this issue yesterday - we were able to make improvements that fixed several buggy cases. I'm still running into a case like below that's going to need more logging to figure out, not sure where the extra 2 is coming from:

about to drop 2 in onesPlayArea return zone
onesPlayArea set to 1, matching objectsPlayArea
matching objectsPlayArea: oldValue: 3, newValue: 1
calculating and setting total in objectsPlayArea: 2
calculating and setting total in objectsPlayArea: 1
calculating and setting total in onesPlayArea: 1
about to add 1 with up arrow in in total accordion box
onesPlayArea set to 4, matching objectsPlayArea
matching objectsPlayArea: oldValue: 1, newValue: 4
Assertion failed:  A play area should not need to create more than one counting objectat a time to match the opposite play area: 3

[chrisklus]

I got some better logging going and set a debugger for the bad cases to look at the stack - there does not seems to be a consistent place/reason for adding numbers that's cause it to get off (the above case was clicking an up arrow in the total accordion box, below is dragging out a 1 and clicking the up arrow in a play area's creator node, which leads me to believe something else is going on. The next step is likely to look at the full state of paper numbers before and after the mystery change.

about to drag one out in in onesPlayArea
calculating and setting total in onesPlayArea: 1
onesPlayArea set to 1, matching objectsPlayArea
matching objectsPlayArea: oldValue: 0, newValue: 1
calculating and setting total in objectsPlayArea: 1
about to drag one out in in onesPlayArea
calculating and setting total in onesPlayArea: 5
onesPlayArea set to 5, matching objectsPlayArea
matching objectsPlayArea: oldValue: 1, newValue: 5
Assertion failed:  A play area should not need to create more than one counting objectat a time to match the opposite play area: 4

objectsPlayArea set to 2, matching onesPlayArea
matching onesPlayArea: oldValue: 3, newValue: 2
calculating and setting total in onesPlayArea: 2
about to drop 2 in onesPlayArea return zone
calculating and setting total in onesPlayArea: 0
onesPlayArea set to 0, matching objectsPlayArea
matching objectsPlayArea: oldValue: 2, newValue: 0
calculating and setting total in objectsPlayArea: 1
calculating and setting total in objectsPlayArea: 0
about to add 1 with up arrow in in onesPlayArea
calculating and setting total in onesPlayArea: 3
matching objectsPlayArea: oldValue: 0, newValue: 3
Assertion failed:  A play area should not need to create more than one counting objectat a time to match the opposite play area: 3

[zepumph]

@chrisklus and I worked on this today, and with this patch we saw that when breaking apart a counting object that is animating to be returned, the animated and includeInSumProperty state is not persevered. This patch helped us find the problem

```diff Index: number-play/js/common/model/CountingPlayArea.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-play/js/common/model/CountingPlayArea.ts b/number-play/js/common/model/CountingPlayArea.ts --- a/number-play/js/common/model/CountingPlayArea.ts (revision d85788a5c69054b0d50e4a319f6eaadacedb3db3) +++ b/number-play/js/common/model/CountingPlayArea.ts (date 1669076238108) @@ -387,6 +387,8 @@ const objectsToBreakDown = [ ...this.countingObjects ]; + const previousCount = _.sum( this.countingObjects.filter( countingObject => countingObject.includeInSumProperty.value ).map( x => x.numberValueProperty.value ) ); + objectsToBreakDown.forEach( countingObject => { if ( countingObject.numberValueProperty.value > 1 ) { const countingObjectPosition = countingObject.positionProperty.value; @@ -420,6 +422,11 @@ } } } ); + + + const newCount = _.sum( this.countingObjects.filter( countingObject => countingObject.includeInSumProperty.value ).map( x => x.numberValueProperty.value ) ); + + assert && assert( previousCount === newCount, 'just, dont' ); } } Index: counting-common/js/common/model/CountingObject.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/counting-common/js/common/model/CountingObject.ts b/counting-common/js/common/model/CountingObject.ts --- a/counting-common/js/common/model/CountingObject.ts (revision a1f0a5df0cb00771295056005297eab2ec89eb38) +++ b/counting-common/js/common/model/CountingObject.ts (date 1669074282677) @@ -70,6 +70,8 @@ groupingEnabledProperty: new BooleanProperty( true ) }, providedOptions ); + console.log( 'the heck!?!? a counting object', new Error().stack ); + // IDs required for map-like lookup, see https://github.com/phetsims/make-a-ten/issues/199 this.id = nextCountingObjectId++;

[chrisklus]

I pushed a fix above and have not been able to hit any assertions when fuzzing. If the repeating problems are removed from CT, I will close. Marking on hold for now.

[chrisklus]

Errors from these changes are no longer coming up on CT, woohoo! Closing.