Open zepumph opened 1 month ago
Yay.
There is trouble in CCK in studio that requires investigation. A Vertex points to an uninstrumented something.
This error is being triggered for archetypes. The PhetioGroups are defined like so, in Circuit.ts
this.wireGroup = new PhetioGroup( ( tandem, startVertex, endVertex ) => {
return new Wire( startVertex, endVertex, this.wireResistivityProperty, tandem );
}, () => createVertices( WIRE_LENGTH ), {
groupElementStartingIndex: GROUP_STARTING_INDEX,
phetioType: PhetioGroup.PhetioGroupIO( CircuitElement.CircuitElementIO ),
tandem: tandem.createTandem( 'wireGroup' )
} );
createVertices
creates uninstrumented vertices. However, during startup, archetypes are broadcast and the first fail is for 'circuitConstructionKitDc.introScreen.model.circuit.wireGroup.archetype'
. Should the Vertex instances for a CircuitElement archetype be instrumented?
I think that makes sense generally. It is basically saying that all circuit elements can depend on an instrumented start/end vertex. What do you think?
Perhaps this is our first instrumented dependency injection for archetypes. Happy to discuss more in person if that is best.
I made some progress in this patch:
Running in studio, I see this assertion error:
Assertion failed: phetioElement not found: circuitConstructionKitDc.introScreen.model.circuit.wireGroup.startVertex
It's saying the archetype is not a dynamic element, since it is statically created, but then it is looking for the archetype for it, so I'm confused how to proceed (if I do understand the problem). Probably synchronous collaboration is the best way forward here.
@samreid and I got to a commit point by reusing createArchetye() logic from PhetioDynamicElementContainer. This worked very well in all forms except what the phetioID of these "extra" archetypes should be. We settled on siblings of the vertextGroup.archetype and prefixed their names with "archetype". @samreid can you give a spot check to the above commits?
Migration rules inbound
Shoot dang. Migration for this one is actually pretty hard. We aren't good at support ignoring initialStateIncompatible with dynamic elements.
This is because we write a rule like "please ignore initialState for batteryGroup.archeteype", and that doesn't successfully bring back the state that was gracefully deleted for "barryerGroup.battery_0".
I did not fully understand the patch above, but this subpatch propagates state correctly and has a Congratulations.
That may work for this case, but let's discuss the lack of support for dynamic element analyzing + report processing.
Over in https://github.com/phetsims/density-buoyancy-common/issues/275, @samreid and I were surprised that there wasn't an assertion preventing
sim.optionalTandem
as the value of a ReferenceIO serialization. Let's add an assertion.