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

Replot when webgl buffer dims don't match canvas dims #2939

Merged
merged 4 commits into from
Aug 22, 2018

Conversation

etpinard
Copy link
Contributor

@etpinard etpinard commented Aug 22, 2018

... as this can unfortunately happen when relayouting to large width/height on some browsers e.g. Chrome 68 and Chrome 64 (that we use on CI).

Note that this doesn't just affect sploms, but all regl-based trace type and presumably all WebGL-trace types.

cc @alexcjohnson

- this can happen when relayouting to large width/height
  on some browsers (e.g. Chrome 68)
@etpinard etpinard added bug something broken status: reviewable labels Aug 22, 2018
@etpinard
Copy link
Contributor Author

'due to browser/WebGL bug.',
'Clearing graph and plotting again.'
].join(' '));
exports.newPlot(gd, gd.data, gd.layout);
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. This may not be the best idea actually. If the canvas and gl buffer dimensions still don't match after the replot, we could have an infinite loop.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure how that would happen, but if you're worried about it we could do something to limit to 1 redraw like

if(!gd._reglRedrawing) {
    gd._reglRedrawing = true;
    return exports.newPlot(...).then(function() { gd._reglRedrawing = false; });
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's essentially when I went for in 85a646f

@etpinard
Copy link
Contributor Author

etpinard commented Aug 22, 2018

New attempt in -> 6fcec6e

Note that, we need to call Plots.supplyDefaults and Plots.doCalcdata after Plots.cleanPlot to have the correct scene references before _module.plot.

fullLayout = gd._fullLayout;
Plots.doCalcdata(gd);
fullLayout._redrawFromWrongGlDimensions = 1;
return drawFramework();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice! Still a substantial win as we don't need to redo all the drawing.

So, there's still recursion here - but it's just back into this function. So instead of a _redrawFromWrongGlDimensions flag on _fullLayout (that we'd probably have to handle more carefully in principle due to relinkPrivateKeys) could we use a local var in the outer scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, done in 3879a41

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one less private key to worry about when we'll 🔪 relinkPrivateKeys.

... to avoid infinite drawFramework loops
@alexcjohnson
Copy link
Collaborator

Thanks for tracking this down and bearing with my iterative comments :) Looks great now. 💃

@etpinard etpinard merged commit 35af715 into master Aug 22, 2018
@etpinard etpinard deleted the gl-context-vs-canvas-wrong-size branch August 22, 2018 20:34
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.

2 participants