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

re-enable all assertions in PhetioObject #48

Closed
samreid opened this issue Dec 26, 2017 · 17 comments
Closed

re-enable all assertions in PhetioObject #48

samreid opened this issue Dec 26, 2017 · 17 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Dec 26, 2017

This will help us identify the following issues:
(a) cases where an event triggers the an event on the same instance
(b) cases where an object is disposed while triggering an event.

From #46

@samreid
Copy link
Member Author

samreid commented Dec 26, 2017

Bayes is not currently operational, so we will have to rely on local testing to help catch issues.

@samreid
Copy link
Member Author

samreid commented Dec 26, 2017

Please see phetsims/equality-explorer#25 for an issue that was created in Equality Explorer about this assertion.

@samreid
Copy link
Member Author

samreid commented Dec 26, 2017

Local Aqua testing for phet brand (with the 3 assertions enabled) indicated issues in these sims:

I included all red sims (even those with seemingly unrelated issues), in case the unrelated issues may be masking this issue.

@samreid
Copy link
Member Author

samreid commented Dec 26, 2017

The problem in build-a-molecule is exactly what we have been talking about in phetsims/scenery#725 and phetsims/scenery#726. The drag event is calling endDrag. This is causing trouble for PhetioObject's assertions because PhetioObject's new assumption is that an event cannot trigger an event on the same object.

Putting it in a setTimeout solves the problem, but may not work well for input event playback.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

I wonder if the problems in #48 (comment) were fixed by commits in #50 ?

samreid added a commit to phetsims/build-a-molecule that referenced this issue Dec 27, 2017
@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

I added the workaround to use PHET_CORE/Timer to invoke the end drag in the next animation frame.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

Next round of testing

  • build-an-atom: cannot dispose while event is in progress

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

After proposing changes to fix build-an-atom style cases, I added another assertion to make sure objects are not disposed twice, and it is triggering in Circuit Construction Kit.

samreid added a commit to phetsims/circuit-construction-kit-common that referenced this issue Dec 27, 2017
@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

Fixed a latent bug in CCK CircuitElement disposal.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

After fixing that, Next round of testing had a "cannot start event while event is in progress" in black box study.

  • circuit-construction-kit-black-box-study

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

I suspect it could be too restrictive to forbid objects from triggering double events on themselves (even though it seems rare).

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

On the other hand, in a previous case and this case catching double triggering revealed an underlying problem in the code, so maybe it is good to keep (until we find a case we cannot circumvent). Committing fix for MomentaryButtonModel shortly.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

On second thought, I'm not convinced the change above is a good move--it works around the problem at hand but when the button becomes disabled and it does not change the value it may be incorrect for the data stream.

samreid added a commit to phetsims/axon that referenced this issue Dec 27, 2017
@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

Better solution committed above.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

Everything is passing aqua fuzz (except for sugar and salt solutions which has namespace problems). I'll test phet-io aqua before committing.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

Everything is green on phet-io aqua. grunt lint-everything is clear, one commit coming up.

@samreid
Copy link
Member Author

samreid commented Dec 27, 2017

The changes in PhetioObject warrant another review by @pixelzoom. I'll create a new issue to make sure it is set for review.

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

No branches or pull requests

1 participant