Skip to content

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Oct 31, 2018

Fixes #3178.
The getAvgPixelByChannel function is revised and now accepts an id to select which gl layer (e.g. focus/pick/context) to be used by different tests.
The new jasmine test, checks for the parcoords context layer to remain visible after a restyle call.
@etpinard
@alexcjohnson

var canvas = d3.select(id).node();

var imgData = readPixel(canvas, 0, 0, canvas.width, canvas.height);
var n = imgData.length * 0.25;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A minor change here:
instead of dividing by 4, we use multiply by 0.25 so that we return rgb fractions and avoid possibility of rounding small values to 0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

shouldn't cause problems but... is there actually a difference between these in js? anyway n should always be an integer, right?

var pixels = new Uint8Array(4 * w * h);

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious. Can you share an example of this happening in JS?

Copy link
Collaborator

Choose a reason for hiding this comment

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

(am I missing something? why didn't that link get turned into a code snippet?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are totally right and I tested this on IE, FF and Chrome too.
In JS there is actually no difference in JS between multiplying by 0.25.
Thanks for the note.

Copy link
Contributor

@etpinard etpinard Nov 1, 2018

Choose a reason for hiding this comment

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

(am I missing something? why didn't that link get turned into a code snippet?)

image

it's rendering fine now. I'll blame that hiccup on Microsoft.

Thanks for the note.

Thank you for double checking!

@etpinard
Copy link
Contributor

etpinard commented Nov 1, 2018

Thanks for the test @archmoj

Let's merge this in 💃

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