phetsims / graphing-quadratics

"Graphing Quadratics" is an educational simulation in HTML5, by PhET Interactive Simulations.
MIT License
1 stars 4 forks source link

Parabola is missing/disappears [safari] #206

Closed Nancy-Salpepi closed 5 months ago

Nancy-Salpepi commented 5 months ago

Test device MacBook Air M1 chip

Operating System 14.4.1

Browser Safari 17.4.1

Problem description See during MR testing (https://github.com/phetsims/qa/issues/1063), but also in published and on main: When opening any of the first 3 screens the parabola is missing, but is present on the Focus and Directrix screen. Changing the value of "a" on any of the first 3 screens causes the parabola to appear and disappear.

I see also see this issue with iPadOS 17.4.1. I don't see this issue with mac + chrome or with Graphing Slope-Intercept or with Graphing Lines.

Steps to reproduce Here is an example:

  1. Go to the Explore Screen--the parabola is missing
  2. Slowly move the slider for "a"

Visuals

https://github.com/phetsims/graphing-quadratics/assets/87318828/0aeb2967-0dfe-4a0e-ba4d-cbf40d9e6c7e

Troubleshooting information: !!!!! DO NOT EDIT !!!!! Name: ‪Graphing Quadratics‬ URL: https://phet.colorado.edu/sims/html/graphing-quadratics/latest/graphing-quadratics_all.html Version: 1.3.5 2024-03-11 17:35:25 UTC Features missing: applicationcache, applicationcache, touch Flags: pixelRatioScaling User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/17.4.1 Safari/605.1.15 Language: en-US Window: 1324x706 Pixel Ratio: 2/1 WebGL: WebGL 1.0 GLSL: WebGL GLSL ES 1.0 (1.0) Vendor: WebKit (WebKit WebGL) Vertex: attribs: 16 varying: 30 uniform: 1024 Texture: size: 16384 imageUnits: 16 (vertex: 16, combined: 32) Max viewport: 16384x16384 OES_texture_float: true Dependencies JSON: {}
KatieWoe commented 5 months ago

My iPad started on 17.3.1 and I did not see this. After updating to 17.4.1 I do see this bug.

pixelzoom commented 5 months ago

I do NOT see this problem on my Intel MacBook Pro (Model identifier = MacBookPro16,1) with macOS 14.4.1 + Safari 17.4.1. This suggests that the problem is specific to M-series chips.

I did NOT see this problem on my wife's MacBook Air M2 with macOS 14.3.1 + Safari 17.3.1. After updating to macOS 14.4.1 + Safari 17.4.1, I DO see this problem. This suggests this problem was introduced in macOS 14.4.

I DO see this problem on my iPad6 with iPadOS 17.4.1. @KatieWoe reported that she did not see the problem until updating from iPadOS 17.3 to 17.4. This suggests that this problem was introduced in iPadOS 17.4.

The quadratic (parabola) and equation that labels the quadratic are drawn by QuadraticNode.ts. The quadratic is a Path, the equation is GQEquationNode. On platforms where this problem occurs, checking the "Equations" checkbox properly displays the equations. So it's only the Path that is not rendering. Here's the method in Quadratic.ts where the Path's Shape is computed:

  private update( quadratic: Quadratic ): void {

    // update shape
    const bezierControlPoints = quadratic.getControlPoints( this.xRange );
    this.quadraticPath.shape = new Shape()
      .moveToPoint( bezierControlPoints.startPoint )
      .quadraticCurveToPoint( bezierControlPoints.controlPoint, bezierControlPoints.endPoint )
      .transformed( this.modelViewTransform.getMatrix() );

    // update color
    this.quadraticPath.stroke = quadratic.color;

    // update equation
    this.updateEquation( quadratic, GQSymbols.yMarkupStringProperty.value, GQSymbols.xMarkupStringProperty.value, GQSymbols.xSquaredMarkupStringProperty.value );
  }

I don't see anything unusual here, so I'll need @jonathanolson to have a look.

Also raising priority to high, since this makes the published version unusable on macOS 14.4.

jonathanolson commented 5 months ago

On it!

kathy-phet commented 5 months ago

@Nancy-Salpepi @KatieWoe - Sounds like you should keep an eye out for additional related issues to this on other sims.

jonathanolson commented 5 months ago

Potential workaround for this on main (committed above). It introduces a slight inefficiency into how SVG shapes containing quadratic segments are done, but I don't see a TON of code usage for this. Potentially could slow down rendering of some of our icons, but probably minimal. Only applied to Safari.

Basically, we mathematically split the quadratic curve in two (efficiently), and tell SVG Safari to render both of those.

Safari seems to be omitting certain quadratic curves on SVG paths. It's particularly problematic since it seems to be somewhat "robust" to perturbation (slightly changing control points doesn't seem to prevent this). Interestingly, the control points of the quadratic are way off-screen (this might be a necessary-but-not-sufficient condition).

I wasn't able to quickly determine any pattern to tweak things into a visually-correct looking way. https://github.com/phetsims/scenery/blob/main/tests/browsers/safari-quadratic.html is a minimal reproduction. https://github.com/phetsims/scenery/blob/main/tests/browsers/safari-quadratic-many.html (many of these parabolas from the sim, lined up together, so working cases will look smooth) shows a lot of irregularity:

Screenshots, representing 'a' values from 0.2 to 3 in the simulation, showing a parabola every 0.001:

Chrome (expected, correct):

image

Safari (broken, but has attitude):

image

I tried degree-elevation to render as a cubic, but Safari is probably converting this back into a quadratic because it doesn't help: https://github.com/phetsims/scenery/blob/main/tests/browsers/safari-quadratic-cubic.html

However subdivision of the quadratic DOES seem to work: https://github.com/phetsims/scenery/blob/main/tests/browsers/safari-quadratic-subdivided.html.

I'll try to get a reproduction over to the WebKit team tomorrow.

@Nancy-Salpepi, @KatieWoe and @kathy-phet, if this looks good with spot-checks, we would presumably want to do some brief regression checks on other sims. Can we confirm that the workaround above is working in main?

Nancy-Salpepi commented 5 months ago

There still seems to be a delay, which results in fragments appearing/disappearing:

https://github.com/phetsims/graphing-quadratics/assets/87318828/91f998e7-7129-4516-9b23-f97b812b77eb

jonathanolson commented 5 months ago

Reproducing on Safari (macOS), on it.

jonathanolson commented 5 months ago

Doesn't even seem like refreshSVGOnNextFrame() seems to help this.

Commit above... splits the quadratic into 4(!) different quadratics, where each one has a non-power-of-2-split point (0, 0.3, 0.5, 0.7, 1), which... seems to just work (especially when combined with the other patch).

@Nancy-Salpepi and @KatieWoe, does this look fixed?

Additionally, I didn't see cases of this issue happening in other sims, so perhaps if this works, I could add the workaround directly to the release branch?

Nancy-Salpepi commented 5 months ago

It is working much better. I do still see a couple of issues when the parabola is narrow.

  1. On the Vertex Form screen, increase the value of 'a' to 5
  2. Grab the Vertex and move it to (1,6) --> the parabola will disappear
  3. On the Explore screen, increase the values of all 3 variables to 6.
  4. Slowly move the slider up and down for 'c'

https://github.com/phetsims/graphing-quadratics/assets/87318828/8c8e215f-af4e-4329-b0e0-58fd62badf6a

https://github.com/phetsims/graphing-quadratics/assets/87318828/455a3b2e-eece-42f4-92ae-2119c6f361c0

pixelzoom commented 5 months ago

In Slack#DM, @jonathanolson said:

Let me know if it would be good to discuss https://github.com/phetsims/graphing-quadratics/issues/206 (the graphing-quadratics Safari issue). It seems like a game of whack-a-mole trying to split the curve to work around it. I might try an additional (or replacement) split somewhat near to where it is being clipped in the sim. Additionally, renderer='canvas' on Safari I believe also works around the issue.

It doesn't seem like there is much Scenery-specific I can do at the moment, especially since the initial approach of splitting the curve once only seems to "improve" things (might back that change out). Would you be ok taking over twiddling/adjusting workarounds, or have thoughts?

Does specifying renderer='canvas' for the Path part of QuadraticNode.ts resolve the problem? If so, that seems like a good interim solution. If not, please clarify what you mean by "renderer='canvas' on Safari I believe also works around the issue".

Better would be for Safari to fix the problem. Have you reported to the WebKit team yet?

... the initial approach of splitting the curve once only seems to "improve" things (might back that change out)

Yes, I would back that out.

pixelzoom commented 5 months ago

I worked on this with @jonathanolson. Switching to renderer: 'canvas' seems to fix the problem. Using 'canvas' on the Path within QuadraticNode (this.quadraticPath) resulted in a lot of additional Canvas instances due to layering. So we switched to 'canvas' for the entire QuadraticNode to minimize the number of Canvas instances created. While performance still seems to be good, we made this workaround specific to Safari, so that other platforms will not have a performance impact:

renderer: platform.safari ? 'canvas' : null

On my old iPad6 running iPadOS 17.4.1, I no longer see the problem, and performance is still great.

@Nancy-Salpepi Please test in 1.4.0-dev.2. Let me know if you see any rendering or performance problems. Assign back to me for next steps.

Nancy-Salpepi commented 5 months ago

@pixelzoom everything looks good in 1.4.0-dev.2. I didn't see any rendering or performance problems using my MacBook Air with safari 17.4.1. Back to you!

pixelzoom commented 5 months ago

Thanks @Nancy-Salpepi.

I'll proceed with patching the 1.3 (published) branch, then deploying an RC for a spot check.

pixelzoom commented 5 months ago

Ready for RC spot-check in https://github.com/phetsims/qa/issues/1065.

Assign back to me for 1.3.7 publication.

Nancy-Salpepi commented 5 months ago

I am no longer able to reproduce on macOS 14.4.1/iPad 9th generation with safari 17.4.1 in 1.3.7-rc.1.

pixelzoom commented 5 months ago

Thanks @Nancy-Salpepi. I'll proceed with publishing 1.3.7. Closing.

pixelzoom commented 5 months ago

Reopening, to verify when 1.3.7 is published.

pixelzoom commented 5 months ago

1.3.7 is published. I tested on iPadOS 17.4.1, but do not have access to macOS 14.4.1 with an M-series processor.

@Nancy-Salpepi could you please verify? Close if OK. Links:

Beware of browser caching, and check About dialog to verify that you're running 1.3.7.

Nancy-Salpepi commented 5 months ago

It still hasn't updated to 1.3.7 (it says there is an update, but when I click 'get update' it is still the old version). I will try again later.

Nancy-Salpepi commented 5 months ago

Parabolas look great. Closing.