Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate why Properties need hasListenerOrderDependencies: true #186

Open
zepumph opened this issue May 29, 2020 · 5 comments
Open

Investigate why Properties need hasListenerOrderDependencies: true #186

zepumph opened this issue May 29, 2020 · 5 comments

Comments

@zepumph
Copy link
Member

zepumph commented May 29, 2020

From phetsims/axon#215, when running with ?shuffleListeners, we get this error: assigning to responsible dev to determine if it is worth fixing:

Uncaught Error: Assertion failed: heavyParticles has not been populated yet
Error: Assertion failed: heavyParticles has not been populated yet
    at window.assertions.assertFunction (http://localhost:8080/assert/js/assert.js:22:13)
    at http://localhost:8080/gas-properties/js/common/model/ParticleSystem.js:109:19
    at listener (http://localhost:8080/axon/js/DerivedProperty.js:73:35)
    at TinyEmitter.emit (http://localhost:8080/axon/js/TinyEmitter.js:69:53)
    at NumberProperty._notifyListeners (http://localhost:8080/axon/js/Property.js:273:25)
    at NumberProperty.set (http://localhost:8080/axon/js/Property.js:171:14)
    at NumberProperty.reset (http://localhost:8080/axon/js/Property.js:336:10)
    at NumberProperty.reset (http://localhost:8080/axon/js/NumberProperty.js:175:11)
    at ParticleSystem.removeAllParticles (http://localhost:8080/gas-properties/js/common/model/ParticleSystem.js:144:41)
    at ParticleSystem.reset (http://localhost:8080/gas-properties/js/common/model/ParticleSystem.js:136:10)
@pixelzoom
Copy link
Contributor

To be considered when a new release branch is created for this sim. And my feeling is that this is incredibly low priority, and would not block a release.

@pixelzoom
Copy link
Contributor

shuffleListeners was replace with listenerOrder, which is now run as a standard CT test. I haven't seen any listenerOrder errors in CT, so closing this issue.

@phet-dev phet-dev reopened this Dec 16, 2023
@phet-dev
Copy link
Contributor

Reopening because there is a TODO marked for this issue.

@pixelzoom
Copy link
Contributor

It looks like the reason why this is no longer a problem in CT is that @zepumph added hasListenerOrderDependencies: true for 3 Properties. While that does silence the problem, it doesn't address the problem -- it's not clear to me why those 3 Properties need to have order depenendencies.

So... I'll change the title of this issue and leave it open to investigate whether those 3 Properties really need hasListenerOrderDependencies: true.

@pixelzoom pixelzoom changed the title support ?shuffleListeners Investigate why Properties need hasListenerOrderDependencies: true Dec 16, 2023
@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 4, 2024

The Properties with hasListenerOrderDependencies: true are:

  • ParticleSystem.ts: this.numberOfHeavyParticlesProperty, this.numberOfLightParticlesProperty
  • DiffusionSettings.ts: this.numberOfParticlesProperty

I poked around a little, and I don't see anything obvious/easy that can be changed. So we will skip this for the 1.1 release #191, unless it causes PhET-iO problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants