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

Enable tablet #11

Merged
merged 5 commits into from
Oct 10, 2018
Merged

Enable tablet #11

merged 5 commits into from
Oct 10, 2018

Conversation

dy
Copy link
Member

@dy dy commented Sep 21, 2018

@etpinard
Copy link
Member

@nicolaskruchten would you mind checking out https://codepen.io/etpinard/pen/wEOyJG on your iPad?

Thanks!

@nicolaskruchten
Copy link

nicolaskruchten commented Sep 21, 2018

I’ll get the iPad checked soon but FYI it doesn’t work on iPhone 7

@etpinard
Copy link
Member

@etpinard
Copy link
Member

compare to https://codepen.io/etpinard/full/GXexQM/ with plotly-latest.

@etpinard
Copy link
Member

@dy things still don't work on iPad.

Here's a screenshot of Chrome on the "latest generation" iPad:

image

Let me know if you need a refresher on how to use Browserstack.

@nicolaskruchten
Copy link

Confirmed on a physical iPad

@dy
Copy link
Member Author

dy commented Sep 30, 2018

Ok, now that works. Thanks guys for the feedback. That took a while to make correct API.

@nicolaskruchten
Copy link

Lmk if there’s a link I can test! :)

@dy
Copy link
Member Author

dy commented Oct 1, 2018

@nicolaskruchten the same link https://codepen.io/etpinard/pen/wEOyJG should work, unless cached
image

@nicolaskruchten
Copy link

Ok so webgl works on both iPad and iPhone, nice job! But... on iPad the surface plot doesn’t show up initially... only if I drag-rotate it. It sometimes flickers on and then disappears leaving just the title and colorbar

@nicolaskruchten
Copy link

So to be clear, the detection bug is fixed, but likely we will get additional iPad bug reports once this is down streamed :)

@etpinard
Copy link
Member

etpinard commented Oct 1, 2018

on iPad the surface plot doesn’t show up initially... only if I drag-rotate it

Right, I noticed the same issue on browserstack.


Is it worth merging and releasing this as part of this week's v1.42.0? Or should we wait for a fix for that flickers issue?

@nicolaskruchten
Copy link

Your call: is it worse to have a message saying "webgl not supported by your browser" even though it is, or to have a partial figure rendered?

@dy
Copy link
Member Author

dy commented Oct 1, 2018

Hm that seems to be gl-plot3d specific issue. I may look into it.

@etpinard
Copy link
Member

etpinard commented Oct 1, 2018

Your call: is it worse to have a message saying "webgl not supported by your browser" even though it is, or to have a partial figure rendered?

Ok. I'd vote for fixing this "well" once and for all. @dy can you try to look at this next?

@dy dy mentioned this pull request Oct 8, 2018
@dy

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@nicolaskruchten

This comment has been minimized.

@dy

This comment has been minimized.

@dy
Copy link
Member Author

dy commented Oct 8, 2018

Since removing dirty flag and gl.clear() calls (forcing scene to draw every frame) keeps on flickering, the only explanation is that requestAnimationFrame is separate from WebGL frames in iOS in some way. Likely some scene frames take longer to draw than actual gl frames, which effects in skipped blank frames. Keeping preserveDrawingBuffer: true for now.

(the pen https://codepen.io/etpinard/pen/wEOyJG seems to be fixed now @nicolaskruchten)

@nicolaskruchten
Copy link

nicolaskruchten commented Oct 8, 2018 via email

@nicolaskruchten
Copy link

While I’m picking at nits though, the image initially flashes on at an odd camera angle then snaps into place. Tapping “zoom” or “pan” locks it back into that odd angle on both devices for some reason

@dy
Copy link
Member Author

dy commented Oct 8, 2018

@nicolaskruchten that seems to be not only ipad issue, I have it on laptop too (clicking zoom/pan).

@nicolaskruchten
Copy link

Ok, great! I’m happy, so long as these changes don’t destabilize other platforms/OSes

scene.js Outdated
@@ -80,7 +80,8 @@ function createScene(options) {
gl = getContext(canvas,
options.glOptions || {
premultipliedAlpha: true,
antialias: true
antialias: true,
preserveDrawingBuffer: true
Copy link
Member

Choose a reason for hiding this comment

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

Could this be preserveDrawingBuffer: isMobile, to not deteriorate performance?

Copy link
Member Author

Choose a reason for hiding this comment

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

@etpinard that does not affect performance in any way, in fact by avoiding redundant redraws it is possible to improve it. I may have a look at refactoring. Fixed.

@etpinard
Copy link
Member

etpinard commented Oct 9, 2018

Ok, great! I’m happy, so long as these changes don’t destabilize other platforms/OSes

All tests are passing. I didn't notice any differences in behaviour while testing out our gl3d mocks.

Waiting on a reply for #11 (comment) by @dy, but this appears like a solid improvement 🥇

@etpinard
Copy link
Member

Thanks @dy !

Merging this in. This will be part of plotly.js v1.42.0

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

Successfully merging this pull request may close these issues.

3 participants