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

Handling disposal of color Properties #692

Closed
jonathanolson opened this issue Oct 19, 2017 · 0 comments
Closed

Handling disposal of color Properties #692

jonathanolson opened this issue Oct 19, 2017 · 0 comments

Comments

@jonathanolson
Copy link
Contributor

Scenery currently only removes listeners from color properties used as fill/stroke during a Display.updateDisplay() (called during sim steps).

This can create issues with the new listener pattern if something like the following is done:

var colorProperty = new axon.Property( 'blue' );
scene.addChild( new scenery.Circle( 10, { fill: colorProperty } ) );
display.updateDisplay();
scene.removeAllChildren();
colorProperty.dispose();

// Assertion failed: tried to removeListener on something that wasn't a listener
display.updateDisplay();

It would be good to discuss if this is a complication we should get used to, or if there is a way of properly handling it, e.g.:

  1. Be able to listen to when a property is disposed, so that we can remove the listener.
  2. Check if the listener exists when trying to remove it (seems like it could allow unintended bugs to be introduced, but is the highest-performance option).
  3. Have Scenery try to synchronously remove the listener. This would be a 300+ LOC change with a good amount of complexity AND a performance hit, so I'd prefer not to do this.

Thoughts?

@jonathanolson jonathanolson self-assigned this Oct 19, 2017
@pixelzoom pixelzoom changed the title Handling disposal of color properties Handling disposal of color Properties Oct 19, 2017
samreid added a commit to phetsims/axon that referenced this issue Oct 19, 2017
samreid added a commit to phetsims/axon that referenced this issue Oct 19, 2017
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

1 participant