phetsims / circuit-construction-kit-common

"Circuit Construction Kit: Basics" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
10 stars 10 forks source link

Pulling out different types of light bulbs leads to assertion error #958

Closed matthew-blackman closed 1 year ago

matthew-blackman commented 1 year ago

Error message: Assertion failed: phetioID already registered: circuitConstructionKitDc.labScreen.view.circuitNode.lightBulbNodeGroup.lightBulbNode_0

To reproduce:

matthew-blackman commented 1 year ago

Found while investigating https://github.com/phetsims/circuit-construction-kit-common/issues/957

Could this be related to https://github.com/phetsims/circuit-construction-kit-common/issues/630#issuecomment-1421527608 in https://github.com/phetsims/circuit-construction-kit-common/issues/630?

matthew-blackman commented 1 year ago

@samreid @arouinfar and I agree that there should be a separate group for realLightBulbNodeGroup in the view, similar to how groups are being used for resistors, extremeResistors and householdObjects.

matthew-blackman commented 1 year ago

@arouinfar ready for testing. If all looks good, this can be closed.

arouinfar commented 1 year ago

Looks great, thanks!

samreid commented 1 year ago

I found in Studio there was an extra bulb socket like so:

image

So I added the commits above which seemed to fix it. Would be good to code review with @matthew-blackman. Was an extra archetype leaking? And testing to make sure the correct circuit element groups still appear in the appropriate circumstances (lab screen vs intro screen, and ac vs dc).

samreid commented 1 year ago

@matthew-blackman and I discovered phetioIsArchetype is undefined for 2 of the light bulb types on Screen 1. We must find out the cause.

samreid commented 1 year ago

The problem is that PhetioObject.initializePhetioObject has a guard and early return if providedOptions.tandem.supplied is false. In this case, when we used the pattern of

    initializeCircuitElementType( ( e: CircuitElement ) => e instanceof LightBulb && e.isReal, this.fixedCircuitElementLayer,
      new PhetioGroup<CircuitElementNode, [ CircuitElement ]>( ( tandem: Tandem, circuitElement: CircuitElement ) => new CCKCLightBulbNode( screenView, this, circuitElement as LightBulb, this.model.isValueDepictionEnabledProperty, this.model.viewTypeProperty, tandem ),
        () => [ this.circuit.realLightBulbGroup.archetype ], {
          phetioType: PhetioGroup.PhetioGroupIO( Node.NodeIO ),
          tandem: circuit.includeLabElements ? tandem.createTandem( 'realLightBulbNodeGroup' ) : Tandem.OPT_OUT,
          supportsDynamicState: false

That part about tandem: circuit.includeLabElements ? tandem.createTandem( 'realLightBulbNodeGroup' ) : Tandem.OPT_OUT, was creating a PhetioObject with a tandem that exists but is supplied: false. Hence it never runs the phetioIsArchetype assignment. Therefore, our solution that avoids creating these groups with Tandem.OPT_OUT is the correct solution. Closing.

samreid commented 1 year ago

Fixed an assertion error that a group appeared twice. Closing.