Closed samreid closed 4 years ago
@samreid mentioned that there was some trouble with ordering while setting the state involving the equipotential lines. I would really like to know why (if?) the new Group strategy causes this error when the old one doesn't.
I have the same question!
After the above commit, we are able to drag out one electric field sensor which correctly propagates its state.
Remaining work to do for Group and its usage in this sim:
static toStateObject( electricFieldSensorNode ) {
validate( electricFieldSensorNode, this.validator );
return {
sensorPhetioID: electricFieldSensorNode.modelElement.tandem.phetioID
};
}
const electricFieldSensor = phet.phetIo.phetioEngine.getPhetioObject( stateObject.sensorPhetioID );
Maybe this:
defaultState: {
sensorPhetioID: model.electricFieldSensors.prototypes.prototype.tandem.phetioID
}
and the last part of
const electricFieldSensorNode = electricFieldSensorNodes.createNextCorrespondingGroupMember( addedElectricFieldSensor, { sensorPhetioID: addedElectricFieldSensor.tandem.phetioID } );
Today we made more good progress here. We refactored Group quite a bit. Instead of create
having a stateObject
, parameter, we made it support var args that can be passed through createGroupMember
. This is nice because it focuses Group.js more on phet core sim code, and less on the state engine usage of it. To support state we made a stateObjectToArgs
method on IOTypes that can be used to map objects from toStateObject over to the args needed in create
. We will see how this goes long term, but I'm happy for the focus shift away from state. I think it makes Group easier to grok.
Though this issue is mainly about CAF, tandem is still on master, and the Group updates effected phet-io-test-sim, so I updated those usages for this commit too.
@samreid, @zepumph, and I thought about moving stateObjectToArgs
from the IO type to where Group is instantiated, but it didn't work smoothly with IO type inheritance. We are going to investigate a mixin solution over in https://github.com/phetsims/tandem/issues/105.
We worked on this more today, next steps will be moving to master. Do we need to fix the pencil button error first?
defaultArguments
to prototypeArguments
, good idea!We made progress by making it so that ElectricPotentialLine doesn't error out if there are no charges:
@samreid, @chrisklus, and I worked heavily on trying to get the electric potential lines instrumneted with group and working with state. We made good progress! Lines are showing up in the downstream sim (although they are not correct) Current things broken with state for electricPotentialLines:
Ideas about a future algorithm to investigate:
electricPotentialLine.js
. @samreid and @chrisklus and I worked on this more today. We are getting very entrenched in the complexities in supporting the equipotential lines in state. The general approach we are working toward is to have the ElectricPotentialLine.js be a mutable object, so that as more charges are added while setting the PhET-iO state, the potential line can gracefully adapt and redraw itself. The issues we are encountering are bountiful, but the most obvious seems to be that marking Properties as "deferred" while setting state, therefore deferring listeners that change important state until later, is causing confusion for electric potential lines.
Here is a patch that has a fair amount of TODOs marked in it, explaining the hacks and shortcommings of the current approach.
Starting with the patch in the preceding comment, I made enough progress that I thought it should be committed. Multiple equipotential lines are transmitting in the state wrapper. They clear when a charge is moved. It works with studio launch. I'm not aware of any bugs in this part. But dragging the voltage labels around is not transmitted.
@zepumph can you please take a look?
@samreid your commits look great. I would love to discuss some of the TODOs left in the issue. Below is a ~80% complete addition of instrumenting the electric potential line view. I think the problem has something to do with disposal.
@samreid, @chrisklus, and I worked on this substantially today. We continued the above patch, and finished getting the view of electric potential lines completed. We also worked out all the phet-io work related TODOs. @samreid it would be great if you could review the above commits, and to recommend any other changes. I created a few other issues that should be investigated as part of this work, as well as https://github.com/phetsims/phet-io/issues/1454 in general. I ended up getting rid of NodeIO subtypes, and using ReferenceIO instead. See https://github.com/phetsims/tandem/issues/106 for more investigation.
Is there anything left but merging to master and closing?
I skimmed the change sets and things looked good. I tested the state wrapper again and it still looks great. I made two minor changes above. But this branch seems like it is ready for master, so I'll merge it now.
I deleted the branch, the last thing for this issue is it would be nice for @zepumph to double check the minor changes above:
RE: createCorrespondingGroupMember, does this mean that we will need another method for handling this function for heterogenous groups?
All else looks good.
RE: createCorrespondingGroupMember, does this mean that we will need another method for handling this function for heterogenous groups?
Yes, it would probably be createCorrespondingHeterogeneousGroupMember
. I'll open an issue for that. Closing here.
In the above commit I renamed the electricFieldSensorGroup member tandem name from "electricFieldSensorGroup" to "electricFieldSensor". Keeping closed.
UPDATE FROM MK: this work extends from https://github.com/phetsims/phet-io/issues/1454. Hopefully we can get dynamic Groups working in Charges And Fields. We will work on a branch while we prototype.