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

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

Merged
merged 3 commits into from
Feb 12, 2020

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Feb 4, 2020

Follow up of and 4546 and #4548, and in regards with a number of comments from users experiencing webgl setup e.g.
#3640 (comment)
#3640 (comment)
#3640 (comment)
#3640 (comment)
this PR uses a different webgl config for preserveDrawingBuffer in the second attempt to create gl3d scene. With this in place the graph should be displayed instead of a webgl error namely
Error: WebGL warning: checkFramebufferStatus: Framebuffer not complete
even if is-mobile module was unable to detect the correct device.

Demo

@plotly/plotly_js
cc: @hoerup

@archmoj archmoj added bug something broken status: reviewable labels Feb 4, 2020
- no need to rebuild options to avoid webgl context loss on multi-scene layouts
} else { // try second time
try {
// invert preserveDrawingBuffer setup which could be resulted from is-mobile not detecting the right device
isMobile = opts.glOptions.preserveDrawingBuffer = !opts.glOptions.preserveDrawingBuffer;
Copy link
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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
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 fix is-mobile?

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

Copy link
Contributor Author

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.

@etpinard
Copy link
Contributor

💃

@archmoj archmoj merged commit 9e864a5 into master Feb 12, 2020
@archmoj archmoj deleted the display-webgl branch February 12, 2020 17:16
@archmoj
Copy link
Contributor Author

archmoj commented Feb 12, 2020

Also to mention this PR fixed #3640 | Demo

@richardoptibrium
Copy link

I'm running 1.54.2 but it's still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants