Skip to content

Detect correct device in the second attempt to create 3-D scene#4549

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

Merged
archmoj merged 3 commits into from
Feb 12, 2020
Merged

Detect correct device in the second attempt to create 3-D scene #4549

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6929a02
correct config for device in the second create 3-D scene try
archmoj Feb 4, 2020
0a2513b
sync isMobile value with preserveDrawingBuffer
archmoj Feb 4, 2020
a26e5e2
warn in case first gl3d scene failed
archmoj Feb 12, 2020
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
38 changes: 30 additions & 8 deletions src/plots/gl3d/scene.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -91,8 +91,9 @@ function Scene(options, fullLayout) {

var proto = Scene.prototype;

proto.tryCreatePlot = function() {
proto.prepareOptions = function() {
var scene = this;

var opts = {
canvas: scene.canvas,
gl: scene.gl,
Expand DownExpand Up@@ -132,20 +133,41 @@ proto.tryCreatePlot = function() {
opts.canvas = STATIC_CANVAS;
}

var failed = 0;
return opts;
};

proto.tryCreatePlot = function() {
var scene = this;

var opts = scene.prepareOptions();

var success = true;

try {
scene.glplot = createPlot(opts);
} catch(e) {
failed++;
try { // try second time to fix issue with Chrome 77 https://github.com/plotly/plotly.js/issues/4233
scene.glplot = createPlot(opts);
} catch(e) {
failed++;
if(scene.staticMode) {
success = false;
} else { // try second time
try {
// invert preserveDrawingBuffer setup which could be resulted from is-mobile not detecting the right device
Lib.warn([
'webgl setup failed possibly due to',
isMobile ? 'disabling' : 'enabling',
'preserveDrawingBuffer config.',
'The device may not be supported by isMobile module!',
'Inverting preserveDrawingBuffer option in second attempt to create webgl scene.'
].join(' '));
isMobile = opts.glOptions.preserveDrawingBuffer = !opts.glOptions.preserveDrawingBuffer;

@etpinardetpinardFeb 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm not sure I like this.

If the "main" problem here is is-mobile not doing its job in some cases, then we should try to fix is-mobile instead of working around it behind the scenes.

At the very (very) least we should Lib.warn something here to let the user (and the devs) know what's going on, and gather some data on the situations where is-mobile is buggy.

To be more robust, what if instead of flipping preserveDrawingBuffer here, we allow users (via a new config option) to set preserveDrawingBuffer to true to essentially override is-mobile when they want?

@archmojarchmojFeb 12, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

To be more robust, what if instead of flipping preserveDrawingBuffer here, we allow users (via a new config option) to set preserveDrawingBuffer to true to essentially override is-mobile when they want?

The users may not know in advance the correct setup for the devices.

@archmojarchmojFeb 12, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Hmm, I'm not sure I like this.

If the "main" problem here is is-mobile not doing its job in some cases, then we should try to fix is-mobile instead of working around it behind the scenes.

At the very (very) least we should Lib.warn something here to let the user (and the devs) know what's going on, and gather some data on the situations where is-mobile is buggy.

Good call. Done in a26e5e2.

@etpinardetpinardFeb 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The users may not know in advance the correct setup for the devices.

Yes, I know that. But do you agree that the best way to fix this problem would be to fixis-mobile?

@etpinardetpinardFeb 12, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

.... but now with the Lib.warn message I guess we can expect some users to tell us about it - which may lead us to fix is-mobile for these cases.

I'm happy with this solution. That Lib.warn message is very clear.

@archmojarchmojFeb 12, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I totally agree we should try fixing is-mobile to help detect the right device.
Still this PR helps detect/by-pass such edge cases.


scene.glplot = createPlot(opts);
} catch(e) {
success = false;
}
}
}

return failed < 2;
return success;
};

proto.initializeGLCamera = function() {
Expand Down