Reconsider dynamic allocation of cups and pipes.

[pixelzoom]

For code review #41 ...

From implementation-notes.md:

Memory Management

• Dynamic Allocation:
• All the cups and pipes are added and removed dynamically as cups are added and removed through the number spinner.
• Therefore, all cups and pipes must be disposed along with their links and listeners.

I would strongly reconsider this approach. There is a well-know maximum number of cups, and it's not a large number (7). So it would be much simpler to allocate the max cups once, then use the ones that are needed.

We went with that approach in Fourier: Making Waves for the harmonics (and amplitude sliders, etc.) after much design discussion, see https://github.com/phetsims/fourier-making-waves/issues/6. The conclusion was that it was actually preferrable to have a fixed number of harmonics in the Studio tree, rather than onstantly-changing groups of harmonics, sliders, etc. And it seems that the same conclusions should apply here. (If not, I'd like to know why.)

[pixelzoom]

Dymanically allocating cups is also affecting the integrity of your model. In IntroModel.ts, it says that meanProperty cannot be a DerivedProperty:

    // This value is derived from the water levels in all the cups, but cannot be modeled as a DerivedProperty since
    // the number of cups varies
    this.meanProperty = new NumberProperty( ...

If you had a static number of cups, then meanProperty could be derived from numberOfCupsProperty and the water levels, instead of having to make this (imo) awful compromise.

[pixelzoom]

You also would not need this involved listener in IntroModel.ts:

 // add/remove water cups and pipes according to number spinner
    const numberOfCupsListener = ( numberOfCups: number ) => {
//... followed by 23 more lines

You'd simply change visibility of cup and pipe nodes based on numberOfCupsProperty, and ignore irrelevant cups and pipes in the model based on numberOfCupsProperty.

[pixelzoom]

As I said in https://github.com/phetsims/mean-share-and-balance/issues/41#issuecomment-1169396433:

The highest-priority issue is https://github.com/phetsims/mean-share-and-balance/issues/60. The choice to do dynamic allocation of pipes and cups introduces a lot of complexity, and forces serious compromises like https://github.com/phetsims/mean-share-and-balance/issues/75. It's also NOT the approach that PhET used in fourier-making-waves, a sim that is very similar in this regard (it has a max of 11 harmonics, while this sim has a max of 7 water cups.). Perhaps the difference in approach is that @amanda-phet is the designer for this sim, which @arouinfar was the designer for fourier-making-waves. But I would seriously reconsider this decision before proceeding further with implementation of this sim.

So raising the priority of this issue to high.

[kathy-phet]

@amanda-phet and @samreid are both out this week. Let's plan to discuss early week of July 5th, when both are back in town. Does that work @marlitas?

[pixelzoom]

Thinking about how to proceed with this issue, my recommendation is (in this order):

(1) Have the MSAB team take a look at FMW in Studio, to review how we addressed a variable number of harmonics. A variable number of water cups could be handled in the same way. Looking at the Discrete screen subtree is sufficient. Here are the important elements, where '*' is a number in the range [1,11]:

fourierMakingWaves.discreteScreen.model.fourierSeries.numberOfHarmonicsProperty fourierMakingWaves.discreteScreen.model.fourierSeries.harmonics.harmonic* fourierMakingWaves.discreteScreen.view.charts.amplitudes.amplitudesChartNode.amplitude*NumberDisplay fourierMakingWaves.discreteScreen.view.charts.amplitudes.amplitudesChartNode.amplitude*Slider

(2) Have a design meeting that includes the MSAB and FMW teams: @amanda-phet, @marlitas, @samreid, @kathy-phet, @arouinfar, and me. Discuss:

• what we did (and why) in FMW, by reviewing https://github.com/phetsims/fourier-making-waves/issues/6
• whether the FMW approach (static instances) is a good/better fit for MSAB
• whether consistency of approaches between sims is a PhET goal
• ??

[kathy-phet]

This is helpful, Chris. Thanks for bringing up the notion of consistency across sims with PhET-iO approach, too. Defintely useful to consider for client experience.

[marlitas]

@kathy-phet that works for me. I'll focus on going through the other review comments and issues until then.

[samreid]

@zepumph and I had a meeting scheduled to work on improving the state for the dynamic PhetioGroup in https://github.com/phetsims/mean-share-and-balance/issues/37 --I'll put that on hold until this issue is resolved.

[samreid]

Today @marlitas @arouinfar @kathy-phet @amanda-phet @pixelzoom and @zepumph and myself @samreid met to discuss this topic.

The general consensus was that PhetioGroup is essential and important for NaturalSelection and Circuit Construction Kit. They have a large number of non-reused items.

We decided we wanted to try statically allocating the 2d cups, 3d cups and pipes for this simulation for the following reasons:

• The studio tree interface will be acceptable. In other sims such as Geometric Optics: Basics, there are items in the tree that are not shown in the sim and that is ok. We want to make this a normal part of our process.
• There is a small number of reusable items
• The implementation/complexity will go down significantly

We demonstrated that indices in PhetioGroup can be reused if you decrement groupElementIndex in disposeElement. However, that is not desirable for Circuit Construction Kit or Natural Selection which should have incrementing non-reused indices. One day we may want to have an option in PhetioGroup that lets you select whether indices would be reused or not.

We discussed ways to hide the non-active elements from the studio tree. We will not be taking action on any of these ideas now.

• Hiding items that are non-active
• Strikethrough items that are non-active
• A hidden-eye icon next to tree items that are non-active.

@zepumph also asked the question about why the cost to use PhetioGroup was high. I feel like numerous tiny errors accumulated quickly. Each error was solved in 30-60 minutes and so didn't seem like that much of a problem in itself (hence did not leave a paper trail), but the fact that we hit maybe 10 problems like that led to the overall costs. Without analyzing the commit history and reflecting, I'm not sure I could identify those problems specifically.

UPDATE: You can see search the mean-share-and-balance commits for the term "group" to see several of the related commits. However, it does not show any of the dead ends we abandoned and never committed.

@pixelzoom can you please review and see if I missed any important details?

[pixelzoom]

Thanks @samreid, excellent summary.

[marlitas]

Beginning to switch dynamic allocation to static allocation. Certain this will cause testing errors in CT. Will address as quickly and thoroughly as possible, while chipping away at this change.

[samreid]

All known problems moved to side issue, everything else looks great here. Nice work @marlitas! We recommend @pixelzoom re-review significant changes like this, but it's not clear whether we should do that one-at-a-time or wait until we are closer to the RC. @pixelzoom what do you recommend?

[pixelzoom]

There's a duplicate phetioID that's preventing this sim from loading in Studio:

Assertion failed:  phetioID already registered: meanShareAndBalance.introScreen.view.ValveNode
assert.js:28 Uncaught Error: Assertion failed: phetioID already registered: meanShareAndBalance.introScreen.view.ValveNode
    at window.assertions.assertFunction (assert.js:28:13)
    at PhetioEngine.phetioObjectAdded (phetioEngine.ts:658:36)
    at Object.addPhetioObject (phetioEngine.ts:1223:20)
    at Tandem.addPhetioObject (Tandem.ts:196:38)
    at ValveNode.initializePhetioObject (PhetioObject.ts:335:17)
    at ValveNode.initializePhetioObject (Node.ts:6321:11)
    at ValveNode.mutate (Node.ts:6311:10)
    at new Node (Node.ts:825:12)
    at new ValveNode (ValveNode.ts:53:5)
    at new IntroScreenView (IntroScreenView.ts:124:13)

So I can't proceed with review of this issue. Please assign back to me when it's fixed.

[marlitas]

Fixed! Assigning back to @pixelzoom

[pixelzoom]

I skimmed the commits and played with the sim in Studio. This all looks very nice.

One question... Has the PhET-iO designer (@amanda-phet?) been consulted about whether to use zero-based numbering for tandems associated with cups and pipes? You're currently using zero-based numbering, where "the first cup" is waterCup2D0. Cups and pipes are not numbered in the UI, so this may be OK. But it's worthing checking with the designer, since it's public-facing and impacts the instructional designer.

In Fourier, we used 1-based numbering. But that was driven by the fact that harmonics are numbered 1...N in the UI. So amplitude A₁ is associated with model.fourierSeries.harmonics.harmonic1, whose index in the array of harmonics is 0.

[marlitas]

Has the PhET-iO designer (@amanda-phet?) been consulted about whether to use zero-based numbering for tandems associated with cups and pipes?

This has not been solidified by design. I believe it was briefly mentioned in the dynamic vs. static meeting about using zero-based numbering, but I don't have documentation to confirm. Assigning to @amanda-phet to get feedback.

[pixelzoom]

Also noting that whether to prefer zero-based vs one-based numbering in PhET-iO tandem names may be a more general discussion. It may be related to who we think the instructional designers will be, and whether they can "think like a programmer" (zero-based) or not (one-based). And of course there will be sim-specific cases (like Fourier) where tandem numbering should match UI numbering, regardless of what the PhET convention is.

[kathy-phet]

@JacquiHayes - Feel free to put on your "instructional designer" hat and comment on numbering in PhET-iO groups in general.

[JacquiHayes]

In static cases, where you have a small number of elements to be named, I feel like the numbers should match the natural numbers (one-based). Cup 1 to Cup 7.

Generally, I assume that our IDs are using platforms that already have a numbering system in play.

For some platforms, the developers will have created zero-based numbering systems, others would have converted their numbering system to one-based. To be pragmatic about it, I assume it is easier for IDs who are using a zero-based to use a one-based system than for an ID who has never heard of a zero-based to learn it.

[samreid]

We agreed months ago to use 0-based indexing unless there is a special case where there is a domain-specific constraint (such as in Fourier). However, (a) I could not find a paper trail for that decision and (b) it could be that we now want to overturn that decision. Perhaps @zepumph knows more? Maybe best to chat in an upcoming zoom meeting since it seems there is disagreement here.

[pixelzoom]

If a convention was established, this developer is not aware of it, and can find no documentation. So I thought it was worth discussing. I also think that the numbering convention should be decided by non-programmers, because programmers (incorrectly) think that non-programmers understand 0-based numbering.

[amanda-phet]

I thought we decided we were ok with 0-based indexing, too. We use it in Natural Selection for the bunnies (something like bunny_0), but maybe in that case it makes sense because it's the "initial" bunny. In this case we are talking about such a small number of cups that maybe we should move ahead with using natural numbers 1-7 for the cups.

[pixelzoom]

Yes, we did decide to default to 0-based indexing. But we also decided to use 1-based indexing where it's a better fit. And that's why this issue was opened, to discuss whether 1-based indexing is a better fit for this sim.

In Natural Selection, bunnies are created dynamically. They are not numbered in the UI, and clients are unlikely to ever inspect the elements under model.bunnyCollection in Studio. So the numbering is unimportant, and we used 0-based numbering (which is sort of the default when programming :)

In Fourier, we used 1-based numbering because the harminonics are specifically numbered in the UI. I.e., A₁, A₂, etc. and 1-based numbering is explicitly used when talking about a Fourier Series.

In MSAB, cups and pipes are not explicitly numbered in the UI. But MSAB does seem to be more aligned with Fourier, in that instructional designers (and teachers, students?) are likely to refer to "the first cup", "the second cup", etc. I don't feel strongly either way, just wanted to make sure that it has been considered.

[amanda-phet]

Let's use 1-based numbering for the cups/plates etc. in this sim.

[marlitas]

All indexing is now 1-based for PhET-iO. Sending back to @amanda-phet and @pixelzoom for review and feedback.

[pixelzoom]

I inspected the Studio tree. It looks great to me!

[marlitas]

The changes in this issue have been applied for a while with no additional feedback. Therefore, closing as completed.