phetsims / buoyancy

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

Better PhET-iO support for Shapes #160

Open zepumph opened 2 weeks ago

zepumph commented 2 weeks ago

https://github.com/phetsims/buoyancy/issues/153 is breaking CT and showing that we are not currently supporting dynamic shape instances well. I'll create a new issue to discuss.

@samreid and I dug up https://github.com/phetsims/phet-io/issues/1454 as we remembered out history with heterogeneous dynamic element containers, most importantly about how we decided not to support them. Here are some quotes:

UPDATE from 11/15/19: We decided against this because PhetioGroups are not necessarily the data structure used in the sim for storing dynamic elements, so instead of making PhetioGroups more complicated by supporting heterogenous groups, we decided it made more sense to create a Group for every type. If you need support for a heterogeneous group, you can create an ObservableArray that holds elements from multiple PhetioGroups by reference.

We outlined two problems that may need attention:

  1. The biggest worry about our current heterogeneous setup is how subtype specific subProperties are not in the API. for example Cuboid.sizeProperty, Cone.heightProperty, Cone.radiusProperty and others.
  2. Let's say that to solve the above, we move all subtype properties into Mass, so we can still have PhetioCapsuleIO, then we need to adjust logic such that the archetype describes a MassIO, and not a subtype, AND we need to make sure that the API validation logic supports creating a subtype phetioTypeName.

Noting here that the assertion over in https://github.com/phetsims/buoyancy/issues/153 isn't just for the type itself. It will also fail out because the radiusProperty etc isn't in the archetype. So more discussion here is needed.

zepumph commented 2 weeks ago

Noting how this should be have originally been done in https://github.com/phetsims/buoyancy/issues/61

zepumph commented 1 week ago

@AgustinVallejo @samreid and I would like to proceed by having a single PhetioCapsule per MassShape per object. So ObjectA gets a whole set of PhetioCapsules, and so does ObjectB. Here is the potential studio tree look:


objects
    objectA
        elementProperty
        capsules
            blockCapsule
            coneCapsule
            etc 
    objectB
        elementProperty
        capsules
            blockCapsule
            coneCapsule
            etc
samreid commented 3 days ago

I considered that center and variability became much simpler when we eliminated the phet-io group, and we designed projectile data lab to have no PhetioGroup or PhetioCapsule. I tested creating each shape statically during startup in this context and saw that in a built sim, the memory footprint was still only 75MB which is acceptable. So this simulation will be better off without any PhetioCapsules at all. After the change, I confirmed that Studio=>Launch works well and preserves the shape selection and customization. Closing.

phet-dev commented 2 days ago

Reopening because there is a TODO marked for this issue.

samreid commented 2 days ago

Moved unrelated TODOs to side issues, closing.

samreid commented 2 days ago

@AgustinVallejo recommended running this past @zepumph so he is aware that the static preallocation was a good solution here. Reopening for review, please close if all is well.