Current Model has plethora of reentrancy issues

[marlitas]

After implementing recommendations from the code review in https://github.com/phetsims/number-pairs/issues/9 @pixelzoom and I noticed that the code was starting to become overly complex, and we were hitting reentrancy issues non-stop. This is specifically related to the following comment: https://github.com/phetsims/number-pairs/issues/9#issuecomment-2383771833

Although having the totalNumberProperty be derived more closely matches the UI experience users are having, it created havoc in the form of multiple proxy Properties, code comments issuing warnings, and non stop buggy interactions. The root cause of this lies in the Number Line Slider's dual behavior when interacting directly with the Slider, and when interacting with the number spinners on the right hand side.

Required Slider Behavior:

• Slider thumb is connected to the leftAddendNumberProperty. When moved it must update this Property accordingly.
• The totalNumberProperty.value should not change when the leftAddendNumberProperty is manipulated.
• The rightAddendNumberProperty.value must increase or decrease to counter the left addend.
• In this scenario, the total is static, the left addend is dynamic, and the right addend is derived.

Required Number Spinner Behavior:

• Each NumberSpinner must update an addend NumberProperty accordingly.
• The left addend number spinner updates both the left addend and the total.
• The right addend number spinner updates both the right addend and the total.
• In this scenarios both the right and left addends are dynamic, and the total is derived.

By forcing a derived totalNumberProperty to cover the above requirements, along with the necessary model interface of ObservableArrays, reentrancy problems quickly become a blocker. One such reentrancy issue that commonly came up in the Cube Representation can be described below:

1. A cube is dragged from one side of the wire to the other. As it passes the addend separator it removes itself from it's current ObservableArray so that it can be added to it's new ObservableArray.
2. The ObservableArray.lengthProperty fires to update the addendNumberProperty
3. The addendNumberProperty link fires to update the addendProxyNumberProperty used for slider
4. The addendProxyNumberProperty link fires to update the ObservableArray according to it's delta change (needed to ensure that the Cube Representation stays in sync with the number line representation).
5. The ObservableArray.lengthProperty now fires again to redo the whole cycle.

This is only one such example of reentrancy. We also hit similar problems that were triggered by the enabledPropertyRange for the Slider, Number Spinners, amongst others.

After discussing with @pixelzoom we determined that the increased complexity of the code, added effort to develop, maintain, and scale did not outweigh the benefit of having the implementation directly match the UI. This provides the largest hurdle in studio, but at best can be disguised with a bidirectional DynamicProperty that acts as a proxy, and at worst addressed via documentation in both studio and examples.md

Therefore we have decided to move forward with the following strategy:

• The leftAddendNumberProperty will be mutable in both number line slider scenarios
• The totalNumberProperty will be mutable in both number line slider scenarios
• The rightAddendNumberProperty will be derived in both number line slider scenarios.

This removes reentrancy issues, simplifies the relationship between NumberProperties and the ObservableArrays, and unites the implementation around most of the screens and representations.

[pixelzoom]

Very nice summary @marlitas. Reminder to add a comment that links to this issue when you revert to making rightAddendNumberProperty the derived Property. And again, apologies for sending you down this path. But I think we both ended up with a better understanding of the implementation requirements for the Sum screen.

[marlitas]

@pixelzoom I dug into the model questions again specifically related to the number spinners and came up with the below patch.... Let me know what you think. Also curious about what you've been thinking about as well.

```diff Subject: [PATCH] NumberSpinner Boolean Flag --- Index: number-pairs/js/sum/view/SumScreenView.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-pairs/js/sum/view/SumScreenView.ts b/number-pairs/js/sum/view/SumScreenView.ts --- a/number-pairs/js/sum/view/SumScreenView.ts (revision 179b2494570c5671a7477807cea08b73cf755939) +++ b/number-pairs/js/sum/view/SumScreenView.ts (date 1729041414365) @@ -18,7 +18,6 @@ import NumberPairsConstants from '../../common/NumberPairsConstants.js'; import { VBox } from '../../../../scenery/js/imports.js'; import AddendSpinnerPanel from './AddendSpinnerPanel.js'; -import NumberProperty from '../../../../axon/js/NumberProperty.js'; type SelfOptions = { //TODO add options that are specific to SumScreenView here @@ -28,7 +27,7 @@ & PickRequired; export default class SumScreenView extends NumberPairsScreenView { - public constructor( model: SumModel, providedOptions: SumScreenViewOptions ) { + public constructor( private readonly model: SumModel, providedOptions: SumScreenViewOptions ) { const options = optionize()( { numberSentenceContent: new NumberSentenceAccordionBox( model, { @@ -54,22 +53,16 @@ super( model, options ); - const leftAddendProxyProperty = new NumberProperty( model.leftAddendNumberProperty.value ); - leftAddendProxyProperty.lazyLink( ( newTotal, oldTotal ) => { - const delta = newTotal - oldTotal; - model.totalNumberProperty.value += delta; - model.leftAddendNumberProperty.value += delta; - } ); - const leftAddendSpinner = new AddendSpinnerPanel( - leftAddendProxyProperty, - model.totalRangeProperty, + model.leftAddendNumberProperty, + model.leftAddendSpinnerRangeProperty, { + leftAddendFiringProperty: model.leftAddendSpinnerFiringProperty, tandem: providedOptions.tandem.createTandem( 'leftAddendSpinner' ) } ); const rightAddendSpinner = new AddendSpinnerPanel( model.totalNumberProperty, - model.totalRangeProperty, + model.rightAddendSpinnerRangeProperty, { tandem: providedOptions.tandem.createTandem( 'rightAddendSpinner' ) } ); Index: number-pairs/js/sum/view/AddendSpinnerPanel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-pairs/js/sum/view/AddendSpinnerPanel.ts b/number-pairs/js/sum/view/AddendSpinnerPanel.ts --- a/number-pairs/js/sum/view/AddendSpinnerPanel.ts (revision 179b2494570c5671a7477807cea08b73cf755939) +++ b/number-pairs/js/sum/view/AddendSpinnerPanel.ts (date 1729041100922) @@ -14,8 +14,12 @@ import Range from '../../../../dot/js/Range.js'; import TReadOnlyProperty from '../../../../axon/js/TReadOnlyProperty.js'; import WithRequired from '../../../../phet-core/js/types/WithRequired.js'; +import optionize from '../../../../phet-core/js/optionize.js'; -type AddendSpinnerPanelOptions = WithRequired; +type SelfOptions = { + leftAddendFiringProperty?: Property | null; +}; +type AddendSpinnerPanelOptions = WithRequired & SelfOptions; export default class AddendSpinnerPanel extends Panel { public constructor( @@ -24,12 +28,46 @@ providedOptions: AddendSpinnerPanelOptions ) { + /** + * TODO: @pixelzoom... what do we think? + * Pass in leftAddendNumberProperty + * + * startInput => update leftSpinner changing flag = true + * + * leftAddend triggers rightDerived + * - rightDerived checks flag + * - updates left addend array + * - updates right addend array + * - If flag === true adjust total by left addend delta as well. + * Triggers right Derived + * - updates right addend array + * + * endInput => update leftSpinner changing flag = false + */ + + const options = optionize()( { + leftAddendFiringProperty: null + }, providedOptions ); + const numberSpinner = new NumberSpinner( addendNumberProperty, totalRangeProperty, { + incrementFunction: ( value: number ) => { + if ( options.leftAddendFiringProperty ) { + options.leftAddendFiringProperty.value = true; + } + return value + 1; + }, + decrementFunction: ( value: number ) => { + if ( options.leftAddendFiringProperty ) { + options.leftAddendFiringProperty.value = true; + } + return value - 1; + }, numberDisplayOptions: { backgroundStroke: null }, tandem: providedOptions.tandem.createTandem( 'numberSpinner' ) } ); + const container = new Node( { children: [ numberSpinner ] } ); Index: number-pairs/js/sum/model/SumModel.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/number-pairs/js/sum/model/SumModel.ts b/number-pairs/js/sum/model/SumModel.ts --- a/number-pairs/js/sum/model/SumModel.ts (revision 179b2494570c5671a7477807cea08b73cf755939) +++ b/number-pairs/js/sum/model/SumModel.ts (date 1729041414377) @@ -21,6 +21,7 @@ import Emitter from '../../../../axon/js/Emitter.js'; import DerivedProperty from '../../../../axon/js/DerivedProperty.js'; import NumberIO from '../../../../tandem/js/types/NumberIO.js'; +import BooleanProperty from '../../../../axon/js/BooleanProperty.js'; type SelfOptions = { //TODO add options that are specific to SumModel here @@ -35,14 +36,17 @@ export default class SumModel extends NumberPairsModel { public override readonly leftAddendNumberProperty: Property; - public readonly leftAddendRangeProperty: TReadOnlyProperty; // The right addend is derived due to competing user interactions in the Sum Screen. // You can find more information in this issue: https://github.com/phetsims/number-pairs/issues/17 public override readonly rightAddendNumberProperty: TReadOnlyProperty; public override readonly totalNumberProperty: Property; - public readonly totalRangeProperty: Property; + // Used to control the enabled/disabled state of the addend spinners. + public readonly rightAddendSpinnerRangeProperty: Property; + public readonly leftAddendSpinnerRangeProperty: Property; + + public readonly leftAddendSpinnerFiringProperty = new BooleanProperty( false ); public readonly inactiveCountingObjects: ObservableArray; public readonly updateRangePropertiesEmitter: Emitter; @@ -67,6 +71,7 @@ return total - leftAddend; }, { + reentrant: true, phetioValueType: NumberIO, tandem: options.tandem.createTandem( 'rightAddendNumberProperty' ) } ); @@ -154,22 +159,36 @@ } ); } - assert && assert( this.leftAddendNumberProperty.value === leftAddendObjects.length, 'leftAddendNumberProperty should match leftAddendObjects length' ); - assert && assert( this.rightAddendNumberProperty.value === rightAddendObjects.length, 'rightAddendNumberProperty should match rightAddendObjects length' ); - assert && assert( leftAddendObjects.length + rightAddendObjects.length === this.totalNumberProperty.value, 'leftAddendObjects.length + rightAddendObjects.length should equal total' ); + // If this link fired because of an update from the leftAddendNumberSpinner we also want to make sure the total + // updates by the same delta. + if ( this.leftAddendSpinnerFiringProperty.value ) { + this.totalNumberProperty.value += leftAddendDelta; + } + + // Because of the reentrancy to the rightAddendNumberProperty above the assertions and rangeProperty updates should + // only fire after the rightAddendNumberProperty has been fully updated. + else { + assert && assert( this.leftAddendNumberProperty.value === leftAddendObjects.length, 'leftAddendNumberProperty should match leftAddendObjects length' ); + assert && assert( this.rightAddendNumberProperty.value === rightAddendObjects.length, 'rightAddendNumberProperty should match rightAddendObjects length' ); + assert && assert( leftAddendObjects.length + rightAddendObjects.length === this.totalNumberProperty.value, 'leftAddendObjects.length + rightAddendObjects.length should equal total' ); + + this.leftAddendSpinnerRangeProperty.value = new Range( NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE.min, NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE.max - rightAddendValue ); + this.rightAddendSpinnerRangeProperty.value = new Range( this.leftAddendNumberProperty.value, NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE.max ); + } + this.leftAddendSpinnerFiringProperty.value = false; } ); leftAddendObjects.lengthProperty.link( leftAddend => { this.leftAddendNumberProperty.value = leftAddend; } ); - this.leftAddendRangeProperty = new Property( new Range( NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE.min, NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE.max - this.rightAddendNumberProperty.value ), { + this.leftAddendSpinnerRangeProperty = new Property( NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE, { phetioValueType: Range.RangeIO, - tandem: options.tandem.createTandem( 'leftAddendRangeProperty' ) + tandem: options.tandem.createTandem( 'leftAddendSpinnerRangeProperty' ) } ); - this.totalRangeProperty = new Property( NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE, { + this.rightAddendSpinnerRangeProperty = new Property( NumberPairsConstants.TWENTY_NUMBER_LINE_RANGE, { phetioValueType: Range.RangeIO, - tandem: options.tandem.createTandem( 'totalRangeProperty' ) + tandem: options.tandem.createTandem( 'rightAddendSpinnerRangeProperty' ) } ); this.updateRangePropertiesEmitter = new Emitter( { Index: sun/js/NumberSpinner.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/sun/js/NumberSpinner.ts b/sun/js/NumberSpinner.ts --- a/sun/js/NumberSpinner.ts (revision be486804416b60e050a8ade4720fd575a330f3f8) +++ b/sun/js/NumberSpinner.ts (date 1729035197353) @@ -293,6 +293,8 @@ // enable/disable arrow buttons const updateEnabled = () => { incrementButton.enabled = ( incrementFunction( numberProperty.value ) <= rangeProperty.value.max ); + console.log( 'enabled update', decrementFunction( numberProperty.value ) ); + console.log( 'range value', rangeProperty.value.min ); decrementButton.enabled = ( decrementFunction( numberProperty.value ) >= rangeProperty.value.min ); }; ```

[pixelzoom]

... Also curious about what you've been thinking about as well.

Sorry, I've been totally consumed with change requests for https://github.com/phetsims/models-of-the-hydrogen-atom/issues/67#issuecomment-2415033501. I'll try to spend some time and review the patch during Thursday meetings.

[pixelzoom]

I just looked through your patch in https://github.com/phetsims/number-pairs/issues/17#issuecomment-2415505787. I think I have the general idea. And I had forgotten about the additional problem of range for NumberSpinner. This makes me more convinced that we should abandon NumberSpinner, and investigate what I described in standup — write our own UI component that consists of and icon + up/down arrows, and takes listeners for the up/down arrows.

[pixelzoom]

Also noting that NumberSpinner is not particularly usable for this age group. The arrow buttons are too small. Something more like NumberPicker's UI would be more appropriate, with big arrow buttons. We have the real estate for this. And I don't think consistency with the other sims in this suite should be the main consideration.

For example... Here's a more age-appropriate UI component, from fraction-comparison:

[pixelzoom]

@marlitas and I discussed this on Zoom.

Problems with using NumberSpinner:

• Our use case does not really involve a NumberProperty, we'd have to do something clever to create a NumberProperty that works (call it cleverNumberProperty).
• PhET-iO will show the NumberSpinner linked to cleverNumberProperty, which is not what we want.
• Description will likely read incorrectly because the value of cleverNumberProperty is not really what's relevant for the spinner.

So while what we need can be thought of as a type of spinner, it's not Property-related, and not number-related. What we want to do is perform an action when the buttons are pressed. For the "left" spinner, something like:

incrementButtonListener: () => {
  leftAddendProperty.value++;
  totalProperty.value++;
},
decrementButtonListener: () => {
  leftAddendProperty.value--;
  totalProperty.value--;
}

For the "right" spinner, something like:

incrementButtonListener: () => {
  totalProperty.value++;
},
decrementButtonListener: () => {
  totalProperty.value--;
}

And for PhET-iO, we'd like to link the "left" spinner to leftAddendProperty, and the "right" spinner to rightAddendProperty.

Extending AccessibleNumberSpinner does not feel appropriate, because (again) we don't really have a NumberProperty. Yes we would get alt input behavior, but we'd get incorrect state description behavior. And not sure about context response behavior.

So... @marlitas is going to put together a quick-and-dirty UI component that consists of an icon (kitty, cube,...) and 2 arrow buttons, with an API for adding listeners to the buttons and providing enabledProperty for the buttons. Then we can evaluate whether this is a good path, and what features we'll need to add.

[marlitas]

I added the new component above. I decided to name it CountingObjectControl, but I never feel confident about naming so happy to change it.

I believe this is the right way to go. It has simplified the model and is currently working with little to no elbow grease. I am running into assertions now and then, but these feel like edge cases we should address rather than foundational issues. Overall I think we have finally found the proper approach in the sim's model architecture.

I would like to discuss the below edge case issues with @pixelzoom to confirm that they are not a deal breaker for the overall implementation we have now.

• In the sum screen, when our total is maxed out (ie. 20) moving the numberline slider causes an assertion. This is a result of how we are handling value changes in the model. The sim is trying to add a counting object to the proper array by removing one from the inactive array (later it would take from the other array to add to the inactive). Because we are maxed out the inactive array is empty. Essentially it is an order dependency problem.
• In all screens with a number line slider, moving the slider quickly back and forth lands the sim in a weird state that causes assertions. I'm assuming this is because changes are coming in quickly and interrupting listeners, but I'm not confident in that assessment.

[pixelzoom]

@marlitas and I met about the edge cases described in https://github.com/phetsims/number-pairs/issues/17#issuecomment-2427514742.

• In the sum screen, when our total is maxed out (ie. 20) moving the numberline slider causes an assertion. ...

Two ideas: (1) When inactiveCountingObjects is empty, get the counting object from the array for the other addend. (2) Populate inactiveCountingObjects with 1 extra object. We prefer (1) because (2) seems clever and will be visible in PhET-iO.

• In all screens with a number line slider, moving the slider quickly back and forth lands the sim in a weird state ...

We suspect that this occurs when the distance that the slider thumb moves results in more than 1 object being added/removed from a ObservableArray. Investigate adding a flag to guard against this case, and/or perhaps investigate whether there's a deeper problem with ObservableArray.

If this doesn't resolve the problem, consult with SR/MK on whether there might be a Timer deferring something to the next animation frame that causes things to get out of order when the thumb is moved rapidly.

[marlitas]

I'm waiting until CT is back up and running to close this issue.

[marlitas]

The model seems to be working well after weeks of working in it. CT is still not 100% clear, but I don't believe the errors are an indication of issues with the architecture. I believe we can close now.