search

phetsims / scenery

Scenery is an HTML5 scene graph.
MIT License
53 stars 12 forks source link

Parts of the UI shift around while dragging something. #1289

Open pixelzoom opened 2 years ago

pixelzoom commented 2 years ago

This has been reported for multiple sims, but I don't see a general scenery issue. Examples:

In ph-scale, we addressed this by adding preventFit: true to a (random!) Node as a workaround. We shouldn't have to do that, and I'm not keen on propagating that workaround to more sims. This should be fixed in scenery.

I have not assigned a priority, but recommend "high". Assigning to @jonathanolson @arouinfar and @kathy-phet to work it out.

Here's a visual example, from https://github.com/phetsims/geometric-optics/issues/213:

panelshift

jonathanolson commented 2 years ago

I'm not able to reproduce https://github.com/phetsims/geometric-optics/issues/213 in macOS 11.5 (Chrome 93). Is there something I can reproduce currently?

Additionally, if it's something related to fitted layers (seems to be what happens a lot), I'm curious what our usage/support for older iPads (e.g. 3rd gen) is. @oliver-phet do you have statistics or a feel of how important performance would be for these? It would potentially be possible to look into profiling and turn off layer fitting if we're not memory-constrained.

pixelzoom commented 2 years ago

I have no problem reproducing phetsims/geometric-optics#213 with unbuilt master on macOS 11.5 + Chrome 94.0.4606.61. Grab the pencil in the picture frame, then move your mouse over the bottom control panel while dragging. Just about everything on the screen moves up and down while I'm doing this.

pixelzoom commented 2 years ago

Looking back at phetsims/ph-scale#226, I see that @jonathanolson also could not reproduce it, while I had no problem reproducing it. Our systems are nearly identical, so I think we need to determine why he can't reproduce this. @jonathanolson let's meet on Zoom.

pixelzoom commented 2 years ago

In https://github.com/phetsims/geometric-optics/issues/213#issuecomment-929625643, @kathy-phet reported:

Note that this jumping is also happening on Windows 10, Chrome Version 94.0.4606.61. It's pretty subtle so I probably wouldn't have noticed it., but I see it if I look closely. It only happens when the object gets closer to the control panel, rather than where it starts and more typical moves from its starting location.

I think its good to track down if we fix the source issue, but I would not let this block a prototype publication.

pixelzoom commented 2 years ago

From https://github.com/phetsims/geometric-optics/issues/213#issuecomment-929669296:

@jonathanolson and I worked on this via Zoom. We're both on MacBook Pros, macOS 11.5, Chrome 94. I see the problem, he doesn't. The only difference we could identify is graphics card -- mine Intel, his Radeon.

@jonathanolson suggested using preventFit: true on the ScreenView. This is a better alternative to layerSplit: true because it won't cause performance problems. I verified that preventFit: true resolved the problem (at least on my system) so I'll proceed with that solution.

As @jonathanolson proposed in https://github.com/phetsims/scenery/issues/1289#issuecomment-929494175, we should perhaps investigate making preventFit: true the default.

pixelzoom commented 2 years ago

@kathy-phet asked me to mention this problem at 9/30/2021 developer meeting, so that all developers are aware of how to address it.

PSA:

samreid commented 2 years ago

We reviewed this today, and @jonathanolson added that we may remove the feature altogether. There has been no discussion of a maintenance release so far--it may not be necessary.

MK: Is it better to put preventFit: true on a sub-node instead of on the entire ScreenView? JO: It depends, and would require performance measurements to see what's best. CM: I can be easiest to put it on the ScreenView so it will be safe. JO: Layer fitting may improve performance, so the underlying canvases/svgs etc only take up part of the screen. It is still up to me to investigate the performance aspect. So it is somewhat performance vs quality. CM: Maybe if the sim is performance constrained, try applying this to leaves and move up the tree? JO: Putting it on a leaf may ripple throughout the entire simulation, so it's not orthogonal like that. There are numerous performance tradeoffs that are difficult to summarize. Maybe start with prevent fit on whatever moves outside of the screen. SR: Can we find out what is causing the pixel-shifting bug, so we can have performance and quality? JO: There is a restriction that if it goes smaller than 2px, there are other aspects that cause visual problems. Rendering something on the 4th pixel of one layer looks different than rendering it on the 6th pixel of another layer which is shifted by 2 pixels. We may use layer fitting on some platforms, but not others. JO: Layer fitting can save time, but also cost time. You don't want to be resizing layers every frame, that would be worse. It's better for static things. MK: How important is this from a design standpoint? CM: As reported by @kathy-phet, we don't want this kind of visual glitch in our finished products. I asked if we could live with putting the workaround in code, the answer was to make the announcement about this problem in the prior comment. It seems we will live with workarounds until we have a better solution.

pixelzoom commented 2 years ago

This has come up again in https://github.com/phetsims/equality-explorer/issues/174, and I'll be applying the same workaround there.

pixelzoom commented 2 years ago

In https://github.com/phetsims/equality-explorer/issues/174, QA discovered some similar "shifting" behavior in Firefox (Mac and Windows) that is not fixed by the preventFit: true workaround.

@jonathanolson investigated, and said in https://github.com/phetsims/equality-explorer/issues/174#issuecomment-974514551:

Confirmed that this looks like SVG just... flickering due to antialiasing changes in Firefox. Testing shape-rendering:geometricPrecision didn't fix. I tried some layerSplit: true cases, which isolated each part (and helped), but it seems impractical.

I don't see a good fix for this (besides WebGL in Scenery wouldn't do this).

So there is no known fix for Firefox at this time.

oliver-phet commented 2 years ago

Sorry for the delay in getting back to this issue -

I'm curious what our usage/support for older iPads (e.g. 3rd gen) is. @oliver-phet do you have statistics or a feel of how important performance would be for these?

iPad 3rd gen only supports up to iOS 9.3.5, which is well below our current system requirements of iOS 13+. Since we aren't supporting those older iPads, I don't think performance on those older devices should impact fixes for this issue!

FYI, I currently can't capture specific device analytics using Google analytics, but looking at our iOS app statistics only 9 of 20,612 sessions over the last month were on iOS 9 (all versions).

pixelzoom commented 2 years ago

Encountering this again in https://github.com/phetsims/function-builder/issues/146, where the recommended workaround is (again) to add preventFit: true. It seems like every time I publish a sim, I'm circling back to this issue, and the preventFit: true workaround.

Raising priority of this issue.

pixelzoom commented 1 year ago

I'm working on beers-law-lab, and seeing this issue there. Guess I'll apply the preventFit: true workaround. That makes 4 sims where I've applied this workaround: beers-law-lab, function-builder, geometric-optics, and ph-scale. This issue has been open for > 1 year, and I can't raise the priority any higher. @jonathanolson @kathy-phet FYI.

jessegreenberg commented 1 year ago

This was noticed in a recent release candidate. In this case it was only observed on a Chromebook. See https://github.com/phetsims/quadrilateral/issues/432. preventFit on a Node with children that frequently move was added as a workaround.

pixelzoom commented 1 year ago

Something like this came up again in https://github.com/phetsims/molecule-polarity/issues/164. We decided not to address it. Since it only occurred on Chromebook, and we were well into the RC process, we concluded that addressing it did not justify the cost and delay.

Nancy-Salpepi commented 7 months ago

Noting that I see this issue in https://github.com/phetsims/qa/issues/1032 and in published Area Model multiplication on Win 10 + FF. When moving the dividers or area handle, the Area Model Calculation panel shifts slightly.

jonathanolson commented 7 months ago

Added a commit above that turns off layer fitting by default at the Display level.

@Nancy-Salpepi, can you see if this helps the general (Chrome) issue? It also seems like there is a Firefox issue happening that this approach doesn't help with (and seems like I can't a-priori fix it until alpenglow is fully operational).

pixelzoom commented 7 months ago

@jonathanolson models-of-the-hydrogen-atom started failing CT (on every cycle) on 1/30/24 @ 7:04PM -- right around the time that you pushed https://github.com/phetsims/scenery/commit/1df1747e64c22d11fa316e16ab05211bca3047d7. Related? 

models-of-the-hydrogen-atom : fuzz : unbuilt
http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/models-of-the-hydrogen-atom/models-of-the-hydrogen-atom_en.html?continuousTest=%7B%22test%22%3A%5B%22models-of-the-hydrogen-atom%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706673484846%22%2C%22timestamp%22%3A1706674707445%7D&brand=phet&ea&fuzz
Query: brand=phet&ea&fuzz
Uncaught Error: Assertion failed: Display was already updating paint, may have thrown an error on the last update
Error: Assertion failed: Display was already updating paint, may have thrown an error on the last update
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/assert/js/assert.js:28:13)
at assert (Display.ts:629:6)
at updateDisplay (Sim.ts:436:21)
at apply (PhetioAction.ts:162:16)
at execute (Sim.ts:1023:30)
at stepSimulation (Sim.ts:1013:11)
at stepOneFrame (Sim.ts:988:11)
[URL] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1706673484846%2Fmodels-of-the-hydrogen-atom%2Fmodels-of-the-hydrogen-atom_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz&duration=90000&testInfo=%7B%22test%22%3A%5B%22models-of-the-hydrogen-atom%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706673484846%22%2C%22timestamp%22%3A1706674707445%7D
[NAVIGATED] http://128.138.93.172/continuous-testing/aqua/html/sim-test.html?url=..%2F..%2Fct-snapshots%2F1706673484846%2Fmodels-of-the-hydrogen-atom%2Fmodels-of-the-hydrogen-atom_en.html&simQueryParameters=brand%3Dphet%26ea%26fuzz&duration=90000&testInfo=%7B%22test%22%3A%5B%22models-of-the-hydrogen-atom%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706673484846%22%2C%22timestamp%22%3A1706674707445%7D
[ATTACHED]
[NAVIGATED] about:blank
[NAVIGATED] http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/models-of-the-hydrogen-atom/models-of-the-hydrogen-atom_en.html?continuousTest=%7B%22test%22%3A%5B%22models-of-the-hydrogen-atom%22%2C%22fuzz%22%2C%22unbuilt%22%5D%2C%22snapshotName%22%3A%22snapshot-1706673484846%22%2C%22timestamp%22%3A1706674707445%7D&brand=phet&ea&fuzz
[CONSOLE] enabling assert
[CONSOLE] continuous-test-load
[CONSOLE] Assertion failed: CanvasNode should not be used with an empty canvasBounds. Please set canvasBounds (or use setCanvasBounds()) on QuadrantNode
[PAGE ERROR] Error: Error: Assertion failed: CanvasNode should not be used with an empty canvasBounds. Please set canvasBounds (or use setCanvasBounds()) on QuadrantNode
at window.assertions.assertFunction (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/assert/js/assert.js:28:13)
at CanvasNodeDrawable.paintCanvas (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/drawables/CanvasNodeDrawable.js:29:15)
at CanvasBlock.renderDrawable (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/CanvasBlock.js:428:14)
at CanvasBlock.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/CanvasBlock.js:179:12)
at BackboneDrawable.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/BackboneDrawable.js:267:33)
at DOMBlock.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/DOMBlock.js:52:22)
at BackboneDrawable.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/BackboneDrawable.js:267:33)
at DOMBlock.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/DOMBlock.js:52:22)
at BackboneDrawable.update (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/BackboneDrawable.js:267:33)
at SimDisplay.updateDisplay (http://128.138.93.172/continuous-testing/ct-snapshots/1706673484846/chipper/dist/js/scenery/js/display/Display.js:440:24)
[CONSOLE] continuous-test-error
...
id: "Sparky Node Puppeteer"
Snapshot from 1/30/2024, 8:58:04 PM
jonathanolson commented 7 months ago

It's possible that it was somehow triggered by that, but I fixed the main issue in https://github.com/phetsims/models-of-the-hydrogen-atom/issues/43. It's a possibility that since it had no canvasBounds, it wasn't drawing before, so the error wasn't triggered?

Nancy-Salpepi commented 7 months ago

Added a commit above that turns off layer fitting by default at the Display level.

@Nancy-Salpepi, can you see if this helps the general (Chrome) issue? It also seems like there is a Firefox issue happening that this approach doesn't help with (and seems like I can't a-priori fix it until alpenglow is fully operational).

I will test when previous fixes for Windows/Mac + Chrome have been reverted. (I tested Area Model Multiplication again and this fix hasn't solved the FF issue). Unassigning for now.

pixelzoom commented 7 months ago

https://github.com/phetsims/scenery/issues/1289#issuecomment-1920441842 understood.

https://github.com/phetsims/models-of-the-hydrogen-atom/issues/43 closed.

Back to @jonathanolson, since this is still labeled "priority:2-high".

jonathanolson commented 7 months ago

I see three different cases here:

  1. Classic "chrome" shifting. Seems related to layer fitting. Layer fitting is now turned off everywhere. I've decided to leave the preventFit:true cases, for utility if we turn it on as a default in the future. This now presumably should NOT happen.
  2. Firefox cases where nothing in the sim is changing size: seems to be preventable by putting layerSplit: true on either (a) the Node that is shifting in the screen, or (b) the Node that is meant to visually move on the screen. This workaround seems like it is working well (but not ideal, and we don't want to layerSplit too often for performance reasons).
  3. Specific cases like area-model, where things ARE actually shifting coordinates in SVG. These ideally should be handled on their own, as I don't see any good general workarounds. For example, inspecting the area-model-multiplication case, CalculationNode is actually changing its background's bounds every frame, and then ALSO shifting its transform to compensate. In an ideal world, these should perfectly cancel out, but Firefox doesn't seem to be doing the ideal behavior. Cases like this should be refactored to avoid the coordinate changes (e.g. have CalculationNode's position be static, and have backgroundBounds have a consistent value where if the contents are small then it won't change size).

Since I believe (1) is solved, and (2) and (3) present workarounds that should be applied in the sims, I'm not sure what else to do with this particular issue. @pixelzoom thoughts?

pixelzoom commented 7 months ago

I've decided to leave the preventFit:true cases, for utility if we turn it on as a default in the future.

I see 42 uses of preventFit: true. All the ones that I inspected in my sims are for addressing this problem, and reference this issue. If the problem is fixed, what's the utility in leaving them in the code?

Since I believe (1) is solved, and (2) and (3) present workarounds that should be applied in the sims, I'm not sure what else to do with this particular issue. @pixelzoom thoughts?

I don't know what to say about (2) and (3), or whether to do anything else. I haven't encountered either of these problems, and I'm not familiar enough with scenery internals.

jonathanolson commented 7 months ago

If the problem is fixed, what's the utility in leaving them in the code?

Because I think there's a non-trivial chance that we'll notice a performance regression in some sim, and we would then potentially go back to having to enabled by default (or more likely, having it turned on by default for some specific sims, and since we've tagged preventFit on some nodes that aren't entire ScreenViews, it could allow some things to be fitted for performance).

I don't necessarily object to having them removed, so if you feel that's the right choice, I'll be ok with it.

pixelzoom commented 7 months ago

OK, let just leave the preventFit: true as they are. They're documented in my sims, so they point to this issue, if we ever need to investigate them.

zepumph commented 7 months ago

Noting here that over in https://github.com/phetsims/scenery/issues/1605, we found that there was a chrome bug in the HighlightOverlay's focusDisplay without layer fitting, so we added it back in.