Skip to content

Fix rendering of WebGL traces on latest Chrome-128 and Edge-128 browsers #7131

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

Fix rendering of WebGL traces on latest Chrome-128 and Edge-128 browsers #7131

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bfabb55
fix Infinity for rendering parcoords on Chrome 128
archmoj Aug 26, 2024
5b6663c
ensure finite uniformMatrix in gl-shader
archmoj Aug 26, 2024
4fcc821
draftlog for PR 7131
archmoj Aug 26, 2024
87c77bf
Merge remote-tracking branch 'origin/master' into parcoords-chrome-128
archmoj Aug 28, 2024
be53698
test webgl jasmine on latest stable chrome i.e. v128 now
archmoj Aug 28, 2024
890ccfd
improve handling of NaNs in parcoords
archmoj Aug 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,26 @@ jobs:
name: Run jasmine tests (part B)
command: .circleci/test.sh virtual-webgl-jasmine

webgl-jasmine-chromeLatest:
docker:
# need '-browsers' version to test in real (xvfb-wrapped) browsers
- image: cimg/node:18.20.4-browsers
environment:
# Alaska time (arbitrary timezone to test date logic)
TZ: "America/Anchorage"
parallelism: 8
working_directory: ~/plotly.js
steps:
- browser-tools/install-browser-tools: &browser-versions
install-firefox: false
install-geckodriver: false
install-chrome: true
- attach_workspace:
at: ~/
- run:
name: Run jasmine tests (part B)
command: .circleci/test.sh webgl-jasmine

flaky-no-gl-jasmine:
docker:
# need '-browsers' version to test in real (xvfb-wrapped) browsers
Expand Down Expand Up @@ -493,6 +513,9 @@ workflows:
- virtual-webgl-jasmine:
requires:
- install-and-cibuild
- webgl-jasmine-chromeLatest:
requires:
- install-and-cibuild
- flaky-no-gl-jasmine:
requires:
- install-and-cibuild
Expand Down
1 change: 1 addition & 0 deletions draftlogs/7131_fix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Fix rendering of parcoords and stackgl traces on the latest Chrome-128 and Edge-128 browsers [[#7131](https://github.com/plotly/plotly.js/pull/7131)]
29 changes: 21 additions & 8 deletions src/traces/parcoords/lines.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,18 @@
'use strict';

// Fix for Chrome 128
var INF = 1e38;
function ensureFinite(arr, dfltNaN) {
return arr.map(function(v) {
return (
isNaN(v) ? dfltNaN :
v > INF ? INF :
v < -INF ? -INF :
v
);
});
}

var vertexShaderSource = [
'precision highp float;',
'',
Expand Down Expand Up @@ -564,14 +577,14 @@ module.exports = function(canvasGL, d) {
return {
maskTexture: maskTexture,
maskHeight: maskHeight,
loA: limits[0].slice(0, 16),
loB: limits[0].slice(16, 32),
loC: limits[0].slice(32, 48),
loD: limits[0].slice(48, 64),
hiA: limits[1].slice(0, 16),
hiB: limits[1].slice(16, 32),
hiC: limits[1].slice(32, 48),
hiD: limits[1].slice(48, 64),
loA: ensureFinite(limits[0].slice(0, 16), -INF),
loB: ensureFinite(limits[0].slice(16, 32), -INF),
loC: ensureFinite(limits[0].slice(32, 48), -INF),
loD: ensureFinite(limits[0].slice(48, 64), -INF),
hiA: ensureFinite(limits[1].slice(0, 16), INF),
hiB: ensureFinite(limits[1].slice(16, 32), INF),
hiC: ensureFinite(limits[1].slice(32, 48), INF),
hiD: ensureFinite(limits[1].slice(48, 64), INF),
};
}

Expand Down
17 changes: 15 additions & 2 deletions stackgl_modules/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23536,6 +23536,19 @@ function makeVector(length, fill) {
return result
}

// Fix for Chrome 128
var INF = 1e38
function ensureFinite(arr) {
return arr.map(function(v) {
return (
isNaN(v) ? 0 :
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this one exclude NaN values? Doesn’t that pose a risk that missing data will be filled in spuriously?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't encounter that case in our tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we have that case (missing data, particularly with lines but also points and surfaces) in our image tests? This is stackgl so I guess my question is scoped to the 3D trace types.

But also, this is different from what you did in parcoords, why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In stackgl jasmine tests I encountered a case with both infinity and NaN. So I had to guard against both.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still want to understand:

  • Why are there differences between the stackgl and parcoords fixes? "because only one has a test that cares about NaN" isn’t enough. Have you tried to find a way to push a NaN into the other one? Does it cause problems?
  • Is there any chance that setting NaN to zero will make missing data suddenly appear? This I would consider a VERY bad bug, so it’s worth looking into carefully. Is there any way you can find to pass in an array containing some null data mixed in with valid data, or a typed array with NaN mixed in, that results in a point appearing where there should be none? If this code is processing the x/y/z arrays for scatter3D for example, I could imagine this happening due to NaN at the beginning, middle, or end of one of the arrays; NaN in all three at once; or different length arrays; all of these coming from either plain arrays or typed arrays. We could enumerate similar sets of conditions for the other trace types, and ideally we should have these conditions in test images so we have confidence the behavior is stable over time. These wouldn't need to all be different images, the same trace can have many incomplete or totally missing data points. And perhaps we already have some of these conditions represented in our test images - if so, fantastic, just point them out to me and we'll be good to go.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The case of NaN didn't happen in the parcoords cases.
But it was present in a mesh3d case.
I start thinking perhaps NaNs could/should be avoided in one layer higher in the stackgl case.
Does that make sense?

Copy link
Contributor

Choose a reason for hiding this comment

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

@archmoj @alexcjohnson is this a must-have for our next minor release?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Given that several trace types seem fully broken on latest Chrome (and Edge, per plotly/plotly.py#4729, not surprising as it’s built on Chrome) I certainly think so. I don’t particularly care how the fix is done but I do want us to test thoroughly that the fix can’t generate incorrect data points on a graph

v > INF ? INF :
v < -INF ? -INF :
v
)
})
}

//Create shims for uniforms
function createUniformWrapper(gl, wrapper, uniforms, locations) {

Expand Down Expand Up @@ -23598,7 +23611,7 @@ function createUniformWrapper(gl, wrapper, uniforms, locations) {
gl['uniform' + d + 'iv'](locations[idx], objPath)
break
case 'v':
gl['uniform' + d + 'fv'](locations[idx], objPath)
gl['uniform' + d + 'fv'](locations[idx], ensureFinite(objPath))
break
default:
throw new GLError('', 'Unrecognized data type for vector ' + name + ': ' + t)
Expand All @@ -23608,7 +23621,7 @@ function createUniformWrapper(gl, wrapper, uniforms, locations) {
if(d < 2 || d > 4) {
throw new GLError('', 'Invalid uniform dimension type for matrix ' + name + ': ' + t)
}
gl['uniformMatrix' + d + 'fv'](locations[idx], false, objPath)
gl['uniformMatrix' + d + 'fv'](locations[idx], false, ensureFinite(objPath))
break
} else {
throw new GLError('', 'Unknown uniform data type for ' + name + ': ' + t)
Expand Down
13 changes: 7 additions & 6 deletions stackgl_modules/package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion stackgl_modules/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
"gl-pointcloud2d": "^1.0.3",
"gl-scatter3d": "^1.4.1",
"gl-select-box": "^1.0.4",
"gl-shader": "4.3.1",
"gl-shader": "github:archmoj/gl-shader#0409f68f8005ef3a3f3fccb6121bc7f0a5fda8bb",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See stackgl/gl-shader#24

"gl-spikes2d": "^1.0.2",
"gl-spikes3d": "^1.0.11",
"gl-streamtube3d": "^1.4.2",
Expand Down