CT: lightParticles has not been populated yet #178
Comments
|
Here's the relevant code in ParticleSystem.js: this.numberOfParticlesProperty = new DerivedProperty(
[ this.numberOfHeavyParticlesProperty, this.numberOfLightParticlesProperty ],
( numberOfHeavyParticles, numberOfLightParticles ) => {
// Verify that particle arrays have been populated before numberOfParticlesProperty is updated.
// If you hit these assertions, then you need to add this listener later. This is a trade-off
// for using plain old Arrays instead of ObservableArray.
assert && assert( this.heavyParticles.length === numberOfHeavyParticles,
'heavyParticles has not been populated yet' );
assert && assert( this.lightParticles.length === numberOfLightParticles,
'lightParticles has not been populated yet' );
return numberOfHeavyParticles + numberOfLightParticles;
}, I'm guessing that this may have something to do with how PhET-iO now restores state. @zepumph @samreid @chrisklus any thoughts? Is there something about how PhET-iO restores state that is relevant here? |
|
I tried simply commenting out the 2 assertion in the above code: // assert && assert( this.heavyParticles.length === numberOfHeavyParticles,
// 'heavyParticles has not been populated yet' );
// assert && assert( this.lightParticles.length === numberOfLightParticles,
// 'lightParticles not been populated yet' );The 2 errors noted by CT go away, but a new error occurs. This one looks like a NumberProperty exceeding its range, not enough of a stack trace to say for sure. |
|
If I run |
pixelzoom
changed the title
|
Noting that this also occurs in diffusion and gasses intro. @pixelzoom edit: Noted in the description above. |
|
Hmm... This Error is also occurring occasionally: |
|
And this variation of "has not been populated yet" in diffusion: |
|
Note that line number for above stack traces have changed, since this was reported before the conversion to ES6 modules. Here's a new stack trace for gas-properties: ... and diffusion: |
|
@samreid helped me with this via Zoom. From #178 (comment):
This is because
What's happening is that the State wrapper is setting this.numberOfParticlesProperty = new DerivedProperty(
[ this.numberOfHeavyParticlesProperty, this.numberOfLightParticlesProperty ],
( numberOfHeavyParticles, numberOfLightParticles ) => {
// Verify that particle arrays have been populated before numberOfParticlesProperty is updated.
// If you hit these assertions, then you need to add this listener later. This is a trade-off
// for using plain old Arrays instead of ObservableArray.
assert && assert( this.heavyParticles.length === numberOfHeavyParticles,
'heavyParticles has not been populated yet' );
assert && assert( this.lightParticles.length === numberOfLightParticles,
'lightParticles has not been populated yet' );
return numberOfHeavyParticles + numberOfLightParticles;
},Since |
|
In the above commit, the assertions are skipped while state is being restored. This will probably result in other similar downstream errors, but resolves this issue. I leave this open until CT verifies that this Error is not longer occurring. |
|
This issue is resolved in CT. As predicted, there's a new downstream issue with the State wrapper, tracking in #181. |
|
Reopening. This same issue needs to be addressed in DiffusionModel. |
|
Addressed in DiffusionModel, closing. |
|
Reopening and removing the workaround that was applied in da92203. Like #182, this stems from a more general problem in PhetioStateEngine. |
pixelzoom
added
status:on-hold
type:automated-testing
and removed
status:ready-for-review
type:bug
labels
|
Over in phetsims/axon#276 we added support to only apply a DerivedProperty's derivation function once when it is deferred, instead of many times with intermediate values even when it was deferred. This fixed this bug with no changes to sim-specific code. The specific lines are in DerivedProperty where |
|
@zepumph This problem does not appear to be fixed, it still occurring in CT: I'm also seeing an Error from the first assertion in the Gas Properties listener, which had not previously occurred: |
|
At one point last night. This bug was fixed, but then we broke it again because this case isn't fixed in the general case like we originally thought. In DerivedProperty, we originally were saying that all dependencies should notify before the derivedProperty would undefer. This turned out to create interdependencies, like in MeasuringTapeNode (found via Projectile Motion testing). Thus the best thing to do for this case IMO was to mark out specific order dependencies that make sure that notifications that populate the particle arrays fire before the DerivedProperty tries to recompute. This doesn't feel horrible to me because of the atypical nature of needing a notification before an undefer. In most cases, we found that order depedencies are waiting on an undefer. If we find that this case is happening a lot, I think it would be good to wrap these calls in a clearer API, but didn't feel too strongly about it just yet. State fuzzing is working locally so I committed. @pixelzoom please review changes to ParticleSystem.js. |
|
Change in ParticleSystem.js looks good, I augmented the documentation a little in the above commit. No problems noted in CT, so closing. Thanks @zepumph! |
After fixing #177 (invalid dt: 0) adding all required tandems in #147, these 2 errors are occurring in CT for gas-properties, gases-intro, and diffusion:
Description of the
phet-io-state-fuzztest in continuous-server.js is:The text was updated successfully, but these errors were encountered: