phetsims / density-buoyancy-common

Common code for the Density and Buoyancy simulations
GNU General Public License v3.0
0 stars 2 forks source link

How should we name the tandems for primary group A and secondary group B? #182

Closed samreid closed 1 week ago

samreid commented 1 month ago

From https://github.com/phetsims/buoyancy/issues/160#issuecomment-2145925951 @zepumph said:

Actually. I don't like the notion of "a" and "b". It should be "primary" and "secondary".

groupA -> primaryMasses aMap -> primaryMassesMap

or whatever you think it best.

Let's consult with @arouinfar

zepumph commented 1 month ago
AgustinVallejo commented 1 month ago

If we follow the naming convention from explore screen we could use shapeA and shapeB. However, in the code they are still referred as primary and secondary, even in the Explore Screen code. If we rename, those should be consistent as well.

zepumph commented 1 month ago
zepumph commented 1 month ago

I also recommend using a class for this structure, so that we don't have to repeat this twice. It could be something like "ShapedMassModel.ts"

zepumph commented 2 weeks ago

working on this now.

zepumph commented 2 weeks ago

Still more to do to get the right phet-io tree structure and renames, but good progress.

AgustinVallejo commented 2 weeks ago

Started replacing but based on the initial comment, decided to stash it, as I'm not totally sure that the decision is. To go from primary to A or viceversa. Personally, I also prefer primary, as MK said originally. PrimarySecondary* is a clearer name than AB* and so on.

So holding for discussion...

AgustinVallejo commented 1 week ago

Addressed I think most of the renamings, there's still some strings, tags and string keys to look into. As well as the three structure

AgustinVallejo commented 1 week ago

Studio tree for each model object:

@zepumph can you please double check that the structure is as it was desired? That item was a bit confusing as there is already a shapeProperty and massProperty in Mass.ts, which were in no way related to the shapeProperty in ShapeModel.ts... could lead to confusions.

zepumph commented 1 week ago

Excellent. I changed the serialization to make it a reference. We don't need a copy of the actual Mass, just a pointer to the one that exists in the shapes collection:

image

I wish that the shapeProperty could be a DerivedProperty, but I sorta gave up because of the complexity of changeShape(). Oh well! Thanks for doing this, I know it was confusing. Ready to close?

AgustinVallejo commented 1 week ago

Ready to close, bye!