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

Add null-data buffer fix for parcoords #2612

Merged
merged 2 commits into from
May 22, 2018
Merged

Conversation

dy
Copy link
Contributor

@dy dy commented May 3, 2018

Related to recent safari null-buffer fixes. Also may facilitate with #2582

@@ -176,7 +176,7 @@ function setAttributes(attributes, sampleCount, points) {
function emptyAttributes(regl) {
var attributes = {};
for(var i = 0; i < 16; i++) {
attributes['p' + i.toString(16)] = regl.buffer({usage: 'dynamic', type: 'float', data: null});
attributes['p' + i.toString(16)] = regl.buffer({usage: 'dynamic', type: 'float', data: new Uint8Array()});
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be new Uint8Array(0) just like you did in:

gl-vis/regl-scatter2d@c1183ef#diff-f0cb24e401db59c6759d5cb577e58cebR403

@alexcjohnson
Copy link
Collaborator

Looks promising - I know that problem is a bit hard to pin down, but are you able to actually see this fix a bug? Also why Uint8Array if I see type: 'float'?

Is there any way we can test this? Like, if we know it's risky to initialize a regl.buffer with null, can we wrap regl.buffer in our whole test suite and throw an error in this case? Similar to strict-d3 I guess, maybe we also need a strict-regl? Would that help us 🔒 some of these other browser-and-hardware-dependent bugs?

@etpinard
Copy link
Contributor

@dy would you mind addressing #2612 (comment) before next Tuesday?

@etpinard
Copy link
Contributor

Is there any way we can test this?

Post regl-project/regl#479, situations like this one are fixed from within regl. So I'd 👎 on adding a strict-d3-esque test for this thing. That said, cross-browser testing for regl is still an open issue (cc regl-project/regl#143) so we still can't guarantee things like this won't happen again. @dy how are you planning on testing regl in different browsers during future development?

@etpinard etpinard added bug something broken status: reviewable labels May 22, 2018
@dy
Copy link
Contributor Author

dy commented May 22, 2018

@etpinard there are 4 CI options: Circle CI, sauce labs, browserstack or browserling. Ideally one of them provides open-source free account.

Also that is troublesome to run all tests directly in a browser. Ideally they need splitting and limiting. Otherwise browsers just hang.

  • refactor tests to run in browsers
  • find free for open-source CI

@etpinard
Copy link
Contributor

Ok. Merging this to be part of 1.38.0 !

@etpinard etpinard merged commit 45e5745 into master May 22, 2018
@etpinard etpinard deleted the fix-parcoords-null-buffer branch May 22, 2018 21:19
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