Skip to content

fix: Update plotly to v2.29, resolve some rendering issues #1806

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

Merged
merged 11 commits into from
Mar 1, 2024

Conversation

mofojed
Copy link
Member

@mofojed mofojed commented Feb 15, 2024

  • v2.28 had some fixes for M1 macs, including for the scattergl issue we've fallen back to scatter for in the past
  • Did not resolve the range breaks issue, so that still requires scatter type
  • See scattergl lines sometimes draws random lines on some hardware plotly/plotly.js#3522 for more details
  • Needed to uninstall plotly, then reinstall it - until I did, line plots were not appearing correctly in scattergl mode
    • Did npm uninstall react-plotly.js --workspace packages/chart and npm uninstall react-plotly.js --workspace packages/chart followed by npm install plotly.js --workspace packages/chart and npm install react-plotly.js --workspace packages/chart
  • Tested on Linux desktop and M1 mac, ensured plots were appearing correctly. Will verify e2e tests as well
  • ScatterGL seems to render things slightly differently, so snapshots needed to be updated for a couple plots

@mofojed mofojed requested a review from mattrunyon February 15, 2024 19:27
@mofojed mofojed self-assigned this Feb 15, 2024
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 46.00%. Comparing base (69e8cdd) to head (23853f4).
Report is 11 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1806      +/-   ##
==========================================
- Coverage   46.11%   46.00%   -0.11%     
==========================================
  Files         628      635       +7     
  Lines       37826    37917      +91     
  Branches     9532     9555      +23     
==========================================
+ Hits        17443    17445       +2     
- Misses      20329    20418      +89     
  Partials       54       54              
Flag Coverage Δ
unit 46.00% <100.00%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@mattrunyon mattrunyon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the e2e tests failed

- v2.28 had some fixes for M1 macs, including for the scattergl issue we've fallen back to scatter for in the past
- Did not resolve the range breaks issue, so that still requires `scatter` type
- See https://github.com/plotly/plotly.js/is
sues/3522 for more details
- Tested on M1 mac, ensured plots were appearing correctly. Will verify e2e tests as well
@mofojed mofojed force-pushed the plotly-version-2.29 branch from 9bb13e5 to f705346 Compare February 26, 2024 18:24
- Some test environments may not support it
@mofojed
Copy link
Member Author

mofojed commented Feb 27, 2024

I'm not sure why it's not rendering lines in scattergl. When I try on codepen with the same data/layout, it all seems to work fine. Perhaps something wrong with the plotly modules we're including??

Error in the logs:

VM4427:327 WebGL: INVALID_OPERATION: useProgram: program not valid
draw @ VM4427:327
VM4427:344 WebGL: INVALID_OPERATION: drawArraysInstancedANGLE: no valid shader program in use

Which looks like https://stackoverflow.com/questions/73403994/plotly-js-with-webgl-over-the-vuejs-app-not-working-webgl-invalid-operation-u
But we're building a custom plotly bundle, instead of including everything...

- For some reason they're not drawing at all. Needs some more investigation...
- Deleted `package-lock.json`, ran `npm run clean`, then ran `npm install` again
- Plotly lines work again in scattergl. I officially hate computers.
@mofojed mofojed requested a review from mattrunyon February 29, 2024 15:47
- Will uninstall and reinstall it, see if that fixes things
- Now it works
@mofojed mofojed merged commit 8892074 into deephaven:main Mar 1, 2024
@mofojed mofojed deleted the plotly-version-2.29 branch March 1, 2024 12:58
@github-actions github-actions bot locked and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants