search

phetsims / number-pairs

"Number Pairs" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 0 forks source link

Model initial sprint #9

Open marlitas opened 2 months ago

marlitas commented 2 months ago

This issue will track commits that will start to form the foundation of the model architecture for Number Pairs.

marlitas commented 1 month ago

Talked with @pixelzoom and gave an overview of this model rough draft. He will review and make recommendations.

pixelzoom commented 1 month ago

Here are my notes from review of the model code. I did not review view code - let me know if you'd like me to do that. In general, this looks like a nice foundation for the model, and I don't see any serious problems.

// The right addend value is the only value that is not required to be directly set by a component controlled
// by the user. Therefore, it is derived from the sum and left addend values.
const rightAddendNumberProperty = new DerivedProperty( [ sumProperty, leftAddendNumberProperty ],

NUMBER_LINE_TEN_RANGE: new Range( 0, TEN_SCENE_RANGE.max ),

NUMBER_LINE_TWENTY_RANGE: new Range( 0, TWENTY_SCENE_RANGE.max )

marlitas commented 1 month ago

@pixelzoom thanks for the review! Some follow up questions.

I don’t understand this in SumModel, line 47. Looking at the mockups, it looks like the UI allows the user to set both leftAddendNumberProperty and rightAddendNumberProperty, and I could expect sumProperty to be derived.

Yeah this is tricky. In NumberPairsModel the totalProperty (used to be sumProperty) needs to be mutable for all the decomposition screens. Would you recommend I override the totalProperty and rightAddendNumberProperty type definition in the SumModel to make the totalProperty derived in that screen?

Or perhaps that documentation shouldn't be in the SumModel?

In NumberPairsConstants, this is redundant, same value as TEN_SCENE_RANGE:

I did that in case in the future there was a decision to change the values for the min or the max in what numbers can be explored in those scenes. For example, if a decision is made that we don't want to include 0 as a number to explore it's decomposition then the TEN_SCENE_RANGE.min would equal 1 and the NUMBER_LINE_TEN_RANGE.min would still need to equal 0. Perhaps that's overkill though? Happy to combine them if that feels better.

In NumberPairsConstants, I don’t understand this. Is this the range of the addends (I’m guessing yes), or the range of the sum? TWENTY_SCENE_RANGE appears to be the range of the sum. Maybe the *_RANGE constant names should qualify what they are the range of?

Hmm It's the range the number line covers in scenes with sums that go up to twenty... Technically that is also the addend range though so I can rename it to ADDEND_TWENTY_RANGE would that be clearer and also more general?

pixelzoom commented 1 month ago

Let's have a synchronous discussion about your followup questions in https://github.com/phetsims/number-pairs/issues/9#issuecomment-2379770640.

pixelzoom commented 1 month ago

@marlitas and I discussed the followup questions.

Re totalProperty et.al. ... A better path forward would be to make totalProperty derived in DecompositionModel, and make radio buttons (currently TotalRadioButtonGroup) set selectedSceneProperty rather than totalProperty. This would also be more consistent with implementation and instrumentation of radio buttons in other sims. It's admittedly less convenient for PhET-iO client to have to select a scene than to set a total, but the price we'd pay for that convenience is large. Better to consider other ways to provide that convenience if it proves to be a pain point for clients.

Re TEN_SCENE_RANGE et.al. ... We renamed to TEN_TOTAL_RANGE, TWENTY_TOTAL_RANGE, TEN_NUMBER_LINE_RANGE, and TWENTY_NUMBER_LINE_RANGE.