-
Notifications
You must be signed in to change notification settings - Fork 4
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
CT Assertion failed: there should be at least as many PDOMDisplays as Displays #46
Comments
This looks PDOM-related. I'm assigning to @jessegreenberg for now, and he and I will likely take a look in our meeting today. |
This may be related:
|
By placing a debugger in PDOMDisplaysInfo, it looks like this happens when the WavesScreenView becomes invisible (switching to the PhotonsScreenView). But I canot produce the bug manually in this way, so something else is important in this state to cause the bug. |
I worked on this for a while but never got to the cause. Some notes though:
I am not sure why this wouldn't be working in the downstream sim and will probably request some assistance from the PhET-iO team. |
We have set |
Coming back to this with @zepumph, the comment to produce the in #46 (comment) is misleading, as we understand it now we still need to fuzz the sim to see this issue. |
We were able to reproduce this problem by checking out these shas, and then fuzzing the state wrapper. https://phet-dev.colorado.edu/html/greenhouse-effect/1.0.0-dev.12/phet/dependencies.json UPDATE: We can also reproduce on master, you just need to add |
@jessegreenberg and I worked on this, but didn't really make much in the way of progress. We found that it isn't specific to the waves screen, or the sunlight button. We reproduced it when setting the micro screen onto a sim currently on the photons screen. We also got the assertion on the Here are two states that triggered this, but I haven't yet tried to just set this on a fresh copy of the sim. I'm not sure they will be helpful, but I want to have them just in case. |
At PhET-iO meeting today we decided this was high priority because it is blocking description on master here. |
As a workaround, just noting here before @jessegreenberg and I start again on this, you can just exclude phet-io state testing on ct with this patch. Then you can add interactiveDescription to this sim and still make progress. Index: data/phet-io-state-unsupported
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/data/phet-io-state-unsupported b/data/phet-io-state-unsupported
--- a/data/phet-io-state-unsupported (revision 87a75701144fab1d5d311b164446c41b158c7733)
+++ b/data/phet-io-state-unsupported (date 1635349914831)
@@ -4,4 +4,5 @@
energy-skate-park
energy-skate-park-basics
fourier-making-waves
+greenhouse-effect
ratio-and-proportion
\ No newline at end of file
|
@jessegreenberg, @jonathanolson and I spent some time on this today and made some real progress! That said, it isn't really good news.. . . Here is a wrapper that can reproduce this consistently by setting two states over 3 seconds. Run this in a directly inside a repo (like studio/html.html). And then run it with We can easily see the paused debugger which demonstrates the issue at hand. Working with @jonathanolson, we found that the issue is because we are undeferring all scenery visibility Properties before notifying any of them. @jonathanolson asserted that scenery is decidedly not set up to support this. It has many assumptions about how the state of the scene graph should be updated after every single operation. In the particular case that the above html file shows, The screenProperty is being notified, which triggers all children to remove their PDOMDisplays, but the ConcentrationSlider is actually supported to be hidden. The runtime thinks that it is visible (not hidden) because the notification step has not occurred for From here we are sorta stuck. @jonathanolson will create a scenery issue to investigate this, and perhaps add more assertions about the assumptions that scenery has about single operations occurring. From there we will either have to decide if all scenery-related Properties will need to notify immediately when undeferring, perhaps by having the PhET-iO state engine support an opt-out for these Properties, or if scenery needs to be more flexible (@jonathanolson was very nervous about this idea, reasonably so). We will proceed in that new issue with some more investigation. Please note that for this particular issue in the above HTML, this patch does fix the immediate problem, but we should not commit this because it is incomplete and just a workaround. Index: js/accessibility/pdom/PDOMDisplaysInfo.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/accessibility/pdom/PDOMDisplaysInfo.js b/js/accessibility/pdom/PDOMDisplaysInfo.js
--- a/js/accessibility/pdom/PDOMDisplaysInfo.js (revision 89b51f05e4bb4c7988912ca2f6e21273a4c6a030)
+++ b/js/accessibility/pdom/PDOMDisplaysInfo.js (date 1635362343461)
@@ -293,13 +293,21 @@
this.pdomDisplays.splice( i, 1 );
}
+ // TODO: we shouldn't check on "effective" children, because of the documentation noted in this constructor
+ const children = this.node._children;
+
// Propagate the change to our children
- for ( i = 0; i < this.node._children.length; i++ ) {
- const child = this.node._children[ i ];
- // NOTE: Since this gets called many times from the RendererSummary (which happens before the actual child
- // modification happens), we DO NOT want to traverse to the child node getting removed. Ideally a better
- // solution than this flag should be found.
- if ( child._pdomDisplaysInfo.canHavePDOMDisplays() && !child._isGettingRemovedFromParent ) {
+ for ( i = 0; i < children.length; i++ ) {
+ const child = children[ i ];
+ // NOTE: RE: _isGettingRemovedFromParent: Since this gets called many times from the RendererSummary (which
+ // happens before the actual child modification happens), we DO NOT want to traverse to the child node
+ // getting removed. Ideally a better solution than this flag should be found.
+ // TODO: canHavePDOMDisplays doesn't seem to cover the case where our child is allowed to have a PDOMDisplay, but doesn't have any pdomInstances at this time.
+ if ( !child.pdomParent && child._pdomDisplaysInfo.canHavePDOMDisplays() && !child._isGettingRemovedFromParent ) {
+
+ // use this.node.getEffectiveChildren()? This is not what pdomDisplays is meant for.
+ // better: !child.pdomParent
+ //child._pdomDisplaysInfo.pdomDisplays.length > 0
child._pdomDisplaysInfo.removePDOMDisplays( displays );
}
}
|
This is going to be worked on in phetsims/scenery#1312. Until this is worked on and fixes this issue, it is recommended to remove this sim from CT PhET-iO state testing when working on PDOM-related features (see https://github.com/phetsims/phet-io/blob/a62d88b335459d3c8815c1555bb73944e95a3052/doc/phet-io-instrumentation-technical-guide.md#turn-off-phet-io-state-testing). Coordinate this change with toggling |
Okay, here are the steps that I took in order to re-enable the a11y view without causing CT failures due to the issue with PCOMDisplays:
@zepumph and @jessegreenberg - Are those the correct steps? |
While what you have listened will work. . . . |
I ran I'm going to leave this open and deferred until phetsims/scenery#1312 is taken care of, and then will follow up with turning state testing back on. |
A fix has been pushed over in phetsims/scenery#1312 (comment), next steps for this issue are to make sure it is being tested on CT in the state wrapper. This will ensure we got the fix correct. While testing for the fix, I noticed some other state issues, so please test fuzzing the state wrapper locally also. |
@zepumph - I need a bit of clarification to help me figure out what the next steps are on this issue. One thing you said was:
Do I need to turn on this testing? If so, how is that done? I just looked around in the code and found the file Also, you said:
I just tried running state fuzz testing locally on my machine and, indeed, it hit an assertion fairly quickly. Is this the sort of thing you're referring to? Do I need to correct all such errors and then turn phet-io state fuzz testing on? |
I see it here: https://github.com/phetsims/perennial/blob/d972159f0e77099bebb3b4ee1ab449fe422d02cb/data/phet-io-state-unsupported#L13 So what I was noting in this issue, is that although the original assertion is in all likelihood fixed, the state wrapper has other bugs in it that prevent me from knowing 100% that this original bug report is solved. Until greenhouse is removed from phet-io-state-unsupported and tested on CT, and the CT is clear of all bugs (including unrelated ones), I would be hesitant to close this issue, since it isn't my repo and isn't my issue. There is no rush on supporting the state wrapper, but if it is possible to be supporting state while implementing other features, it may save time in the long run. Does that make sense? |
I took a little time today to see if I could resolve the errors that this sim is encountering when running the state wrapper test. Unfortunately, the first problem it hit in each of my test runs was a general problem described in https://github.com/phetsims/phet-io/issues/1858, so I think I'm out of luck for working on this at the moment. I'm going to defer this issue for a while, and will take another look in a couple of weeks. |
Sorry about that. We expect https://github.com/phetsims/phet-io/issues/1858 to be fixed now. The remaining assertion
was still present but caused by something else. I have disabled Voicing for GasConcentrationAlerter for now. |
After the fix the @jessegreenberg described above (thanks!) I tested and found that errors were being encountered in the "Options" dialog. I fixed these by opting a lot of the controls in this dialog out of registration, since it's temporary anyway. After this I was able to run fuzz testing on the state wrapper for 10 minutes without any issues, so I have removed the sim from the list of those that don't support phet-io state. I believe that this turns on state testing for it. I'll leave this issue open for a while and will close it if it succeeds in the next several rounds of CT. |
Closing for phetsims/qa#832 as this seems solved. If this is in error, feel free to close. |
The text was updated successfully, but these errors were encountered: