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

Parabola is missing/disappears [safari] #206

Closed
Tracked by #1065
Nancy-Salpepi opened this issue Mar 26, 2024 · 20 comments
Closed
Tracked by #1065

Parabola is missing/disappears [safari] #206

Nancy-Salpepi opened this issue Mar 26, 2024 · 20 comments

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
14.4.1

Browser
Safari 17.4.1

Problem description
See during MR testing (phetsims/qa#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

parabola.mp4
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
Copy link
Contributor

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
Copy link
Contributor

pixelzoom commented Apr 2, 2024

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
Copy link
Contributor

On it!

@kathy-phet
Copy link

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

jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 3, 2024
jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 3, 2024
jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 3, 2024
jonathanolson added a commit to phetsims/scenery that referenced this issue Apr 3, 2024
…Casteljau subdivision (one level) on quadratic bezier curves in Safari, see phetsims/graphing-quadratics#206
jonathanolson added a commit to phetsims/kite that referenced this issue Apr 3, 2024
…Casteljau subdivision (one level) on quadratic bezier curves in Safari, see phetsims/graphing-quadratics#206
@jonathanolson
Copy link
Contributor

jonathanolson commented Apr 3, 2024

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
Copy link
Author

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

parabolaPieces.mp4

@jonathanolson
Copy link
Contributor

Reproducing on Safari (macOS), on it.

@jonathanolson
Copy link
Contributor

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
Copy link
Author

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'
VertexFormScreen.mp4
exploreScreen2.mp4

@Nancy-Salpepi
Copy link
Author

@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
Copy link
Contributor

Thanks @Nancy-Salpepi.

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

pixelzoom pushed a commit that referenced this issue Apr 5, 2024
pixelzoom added a commit that referenced this issue Apr 5, 2024
(cherry picked from commit ad3925b)
@pixelzoom
Copy link
Contributor

pixelzoom commented Apr 5, 2024

Ready for RC spot-check in phetsims/qa#1065.

Assign back to me for 1.3.7 publication.

@pixelzoom pixelzoom assigned Nancy-Salpepi and unassigned pixelzoom Apr 5, 2024
@Nancy-Salpepi
Copy link
Author

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
Copy link
Contributor

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

@pixelzoom
Copy link
Contributor

Reopening, to verify when 1.3.7 is published.

@pixelzoom pixelzoom reopened this Apr 8, 2024
@pixelzoom
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

Parabolas look great. Closing.

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

5 participants