Skip to content

Commit

Permalink
fix: dx now respects the webgl flag (#934)
Browse files Browse the repository at this point in the history
Fixes #612 

The current implementation has the following behavior, although note
that this will not work fully until
deephaven/web-client-ui#2255 is merged
1. If webgl is enabled, nothing happens
2. If webgl is disabled and the webgl chart trace type can be replaced,
it will be (such as `scattergl` -> `scatter`) and replaced again if
reenabled (`scatter` -> `scattergl`)
3. If webgl is disabled and the webgl chart trace can't be replaced
(such as `scatter3d` or `scattermapbox`) the prompt in the image below
will appear. Once accepted, the acceptance persists for this specific
chart, even if webgl is toggled again.
Note that disabling and reenabling through the settings without clicking
Continue doesn't count as an acceptance, so if the prompt appears, the
setting is enabled, then the setting is disabled again, the prompt will
still appear.


![image](https://github.com/user-attachments/assets/75e81f41-b629-44f7-81f1-9ceaeb69d713)

---------

Co-authored-by: Matthew Runyon <[email protected]>
  • Loading branch information
jnumainville and mattrunyon authored Nov 23, 2024
1 parent 075381e commit 9cdf1ee
Show file tree
Hide file tree
Showing 8 changed files with 1,079 additions and 701 deletions.
1,449 changes: 762 additions & 687 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion plugins/plotly-express/setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ package_dir=
=src
packages=find_namespace:
install_requires =
deephaven-core>=0.36.0
deephaven-core>=0.37.0
deephaven-plugin>=0.6.0
plotly
deephaven-plugin-utilities>=0.0.2
Expand Down
21 changes: 11 additions & 10 deletions plugins/plotly-express/src/js/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,8 @@
"update-dh-packages": "node ../../../../tools/update-dh-packages.mjs"
},
"devDependencies": {
"@deephaven/jsapi-types": "1.0.0-dev0.35.2",
"@deephaven/jsapi-types": "1.0.0-dev0.36.1",
"@deephaven/test-utils": "0.97.0",
"@types/deep-equal": "^1.0.1",
"@types/plotly.js": "^2.12.18",
"@types/plotly.js-dist-min": "^2.3.1",
Expand All @@ -51,15 +52,15 @@
"react-dom": "^17.0.2"
},
"dependencies": {
"@deephaven/chart": "0.88.0",
"@deephaven/components": "0.88.0",
"@deephaven/dashboard": "0.88.0",
"@deephaven/dashboard-core-plugins": "0.88.0",
"@deephaven/icons": "0.88.0",
"@deephaven/jsapi-bootstrap": "0.88.0",
"@deephaven/log": "0.88.0",
"@deephaven/plugin": "0.88.0",
"@deephaven/utils": "0.88.0",
"@deephaven/chart": "0.97.0",
"@deephaven/components": "0.97.0",
"@deephaven/dashboard": "0.97.0",
"@deephaven/dashboard-core-plugins": "0.97.0",
"@deephaven/icons": "0.97.0",
"@deephaven/jsapi-bootstrap": "0.97.0",
"@deephaven/log": "0.97.0",
"@deephaven/plugin": "0.97.0",
"@deephaven/utils": "0.97.0",
"deep-equal": "^2.2.1",
"nanoid": "^5.0.7",
"plotly.js": "^2.29.1",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import type { Layout } from 'plotly.js';
import { dh as DhType } from '@deephaven/jsapi-types';
import { TestUtils } from '@deephaven/utils';
import { TestUtils } from '@deephaven/test-utils';
import { ChartModel } from '@deephaven/chart';
import { PlotlyExpressChartModel } from './PlotlyExpressChartModel';
import { PlotlyChartWidgetData } from './PlotlyExpressChartUtils';
Expand Down Expand Up @@ -262,4 +262,62 @@ describe('PlotlyExpressChartModel', () => {
new CustomEvent(ChartModel.EVENT_DOWNSAMPLEFAILED)
);
});

it('should swap replaceable WebGL traces without blocker events if WebGL is disabled or reenabled', async () => {
const mockWidget = createMockWidget([SMALL_TABLE], 'scattergl');
const chartModel = new PlotlyExpressChartModel(
mockDh,
mockWidget,
jest.fn()
);

const mockSubscribe = jest.fn();
await chartModel.subscribe(mockSubscribe);
await new Promise(process.nextTick); // Subscribe is async
chartModel.setRenderOptions({ webgl: true });
expect(chartModel.plotlyData[0].type).toBe('scattergl');
chartModel.setRenderOptions({ webgl: false });
expect(chartModel.plotlyData[0].type).toBe('scatter');
chartModel.setRenderOptions({ webgl: true });
expect(chartModel.plotlyData[0].type).toBe('scattergl');

// No events should be emitted since the trace is replaceable
expect(mockSubscribe).toHaveBeenCalledTimes(0);
});

it('should emit blocker events only if unreplaceable WebGL traces are present and WebGL is disabled, then blocker clear events when reenabled', async () => {
const mockWidget = createMockWidget([SMALL_TABLE], 'scatter3d');
const chartModel = new PlotlyExpressChartModel(
mockDh,
mockWidget,
jest.fn()
);

const mockSubscribe = jest.fn();
await chartModel.subscribe(mockSubscribe);
await new Promise(process.nextTick); // Subscribe is async
chartModel.setRenderOptions({ webgl: true });
// no calls because the chart has webgl enabled
expect(mockSubscribe).toHaveBeenCalledTimes(0);
chartModel.setRenderOptions({ webgl: false });
// blocking event should be emitted
expect(mockSubscribe).toHaveBeenCalledTimes(1);
expect(mockSubscribe).toHaveBeenLastCalledWith(
new CustomEvent(ChartModel.EVENT_BLOCKER)
);
chartModel.setRenderOptions({ webgl: true });
// blocking clear event should be emitted, but this doesn't count as an acknowledge
expect(mockSubscribe).toHaveBeenCalledTimes(2);
expect(mockSubscribe).toHaveBeenLastCalledWith(
new CustomEvent(ChartModel.EVENT_BLOCKER_CLEAR)
);
expect(chartModel.hasAcknowledgedWebGlWarning).toBe(false);
// if user had accepted the rendering (simulated by fireBlockerClear), no EVENT_BLOCKER event should be emitted again
chartModel.fireBlockerClear();
chartModel.setRenderOptions({ webgl: false });
expect(mockSubscribe).toHaveBeenCalledTimes(3);
expect(mockSubscribe).toHaveBeenLastCalledWith(
new CustomEvent(ChartModel.EVENT_BLOCKER_CLEAR)
);
});
});
55 changes: 55 additions & 0 deletions plugins/plotly-express/src/js/src/PlotlyExpressChartModel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,22 @@ import type { Layout, Data, PlotData, LayoutAxis } from 'plotly.js';
import type { dh as DhType } from '@deephaven/jsapi-types';
import { ChartModel, ChartUtils } from '@deephaven/chart';
import Log from '@deephaven/log';
import { RenderOptions } from '@deephaven/chart/dist/ChartModel';
import {
DownsampleInfo,
PlotlyChartWidgetData,
areSameAxisRange,
downsample,
getDataMappings,
getPathParts,
getReplaceableWebGlTraceIndices,
getWidgetData,
isAutoAxis,
isLineSeries,
isLinearAxis,
removeColorsFromData,
setWebGlTraceType,
hasUnreplaceableWebGlTraces,
} from './PlotlyExpressChartUtils';

const log = Log.module('@deephaven/js-plugin-plotly-express.ChartModel');
Expand Down Expand Up @@ -116,6 +120,17 @@ export class PlotlyExpressChartModel extends ChartModel {

isDownsamplingDisabled = false;

/**
* Set of traces that are originally WebGL and can be replaced with non-WebGL traces.
* These need to be replaced if WebGL is disabled and re-enabled if WebGL is enabled again.
*/
webGlTraceIndices: Set<number> = new Set();

/**
* The WebGl warning is only shown once per chart. When the user acknowledges the warning, it will not be shown again.
*/
hasAcknowledgedWebGlWarning = false;

override getData(): Partial<Data>[] {
const hydratedData = [...this.plotlyData];

Expand Down Expand Up @@ -207,6 +222,41 @@ export class PlotlyExpressChartModel extends ChartModel {
this.widget = undefined;
}

override setRenderOptions(renderOptions: RenderOptions): void {
this.handleWebGlAllowed(renderOptions.webgl, this.renderOptions?.webgl);
super.setRenderOptions(renderOptions);
}

/**
* Handle the WebGL option being set in the render options.
* If WebGL is enabled, traces have their original types as given.
* If WebGL is disabled, replace traces that require WebGL with non-WebGL traces if possible.
* Also, show a dismissible warning per-chart if there are WebGL traces that cannot be replaced.
* @param webgl The new WebGL value. True if WebGL is enabled.
* @param prevWebgl The previous WebGL value
*/
handleWebGlAllowed(webgl = true, prevWebgl = true): void {
setWebGlTraceType(this.plotlyData, webgl, this.webGlTraceIndices);

const needsBlocker = hasUnreplaceableWebGlTraces(this.plotlyData);

// If WebGL is disabled and there are traces that require WebGL, show a warning that is dismissible on a per-chart basis
if (needsBlocker && !webgl && !this.hasAcknowledgedWebGlWarning) {
this.fireBlocker([
'WebGL is disabled but this chart cannot render without it. Check the Advanced section in the settings to enable WebGL or click below to render with WebGL for this chart.',
]);
} else if (webgl && !prevWebgl && needsBlocker) {
// clear the blocker but not the acknowledged flag in case WebGL is disabled again
this.fireBlockerClear(false);
}
}

override fireBlockerClear(isAcknowledged = true): void {
super.fireBlockerClear();
this.hasAcknowledgedWebGlWarning =
isAcknowledged || this.hasAcknowledgedWebGlWarning;
}

updateLayout(data: PlotlyChartWidgetData): void {
const { figure } = data;
const { plotly } = figure;
Expand Down Expand Up @@ -246,6 +296,11 @@ export class PlotlyExpressChartModel extends ChartModel {
);
}

// Retrieve the indexes of traces that require WebGL so they can be replaced if WebGL is disabled
this.webGlTraceIndices = getReplaceableWebGlTraceIndices(this.plotlyData);

this.handleWebGlAllowed(this.renderOptions?.webgl);

newReferences.forEach(async (id, i) => {
this.tableDataMap.set(id, {}); // Plot may render while tables are being fetched. Set this to avoid a render error
const table = (await references[i].fetch()) as DhType.Table;
Expand Down
116 changes: 116 additions & 0 deletions plugins/plotly-express/src/js/src/PlotlyExpressChartUtils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import {
removeColorsFromData,
getDataMappings,
PlotlyChartWidgetData,
getReplaceableWebGlTraceIndices,
hasUnreplaceableWebGlTraces,
setWebGlTraceType,
} from './PlotlyExpressChartUtils';

describe('getDataMappings', () => {
Expand Down Expand Up @@ -202,3 +205,116 @@ describe('areSameAxisRange', () => {
expect(areSameAxisRange(null, [0, 10])).toBe(false);
});
});

describe('getReplaceableWebGlTraceIndexes', () => {
it('should return the indexes of any trace with gl', () => {
expect(
getReplaceableWebGlTraceIndices([
{
type: 'scattergl',
},
{
type: 'scatter',
},
{
type: 'scatter3d',
},
{
type: 'scattergl',
},
])
).toEqual(new Set([0, 3]));
});

it('should return an empty set if there are no traces with gl', () => {
expect(
getReplaceableWebGlTraceIndices([
{
type: 'scatter',
},
{
type: 'scatter3d',
},
])
).toEqual(new Set());
});
});

describe('hasUnreplaceableWebGlTraces', () => {
it('should return true if there is a single unreplaceable trace', () => {
expect(
hasUnreplaceableWebGlTraces([
{
type: 'scattermapbox',
},
])
).toBe(true);
});

it('should return true if there are unreplaceable traces', () => {
expect(
hasUnreplaceableWebGlTraces([
{
type: 'scattergl',
},
{
type: 'scatter',
},
{
type: 'scatter3d',
},
{
type: 'scattergl',
},
{
type: 'scatter3d',
},
])
).toBe(true);
});

it('should return false if there are no unreplaceable traces', () => {
expect(
hasUnreplaceableWebGlTraces([
{
type: 'scatter',
},
{
type: 'scattergl',
},
])
).toBe(false);
});
});

describe('setWebGlTraceType', () => {
it('should set the trace type to gl if webgl is enabled', () => {
const data: Plotly.Data[] = [
{
type: 'scatter',
},
{
type: 'scatter',
},
];
const webGlTraceIndices = new Set([1]);
setWebGlTraceType(data, true, webGlTraceIndices);
expect(data[0].type).toBe('scatter');
expect(data[1].type).toBe('scattergl');
});

it('should remove the gl from the trace type if webgl is disabled', () => {
const data: Plotly.Data[] = [
{
type: 'scatter',
},
{
type: 'scattergl',
},
];
const webGlTraceIndices = new Set([1]);
setWebGlTraceType(data, false, webGlTraceIndices);
expect(data[0].type).toBe('scatter');
expect(data[1].type).toBe('scatter');
});
});
Loading

0 comments on commit 9cdf1ee

Please sign in to comment.