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

regl-scatter2d bug fix - to keep trace colors #3242

Closed
wants to merge 6 commits into from
Closed

Conversation

archmoj
Copy link
Contributor

@archmoj archmoj commented Nov 13, 2018

Fixes #3232 in the regl-scatter2d module.
@etpinard
@alexcjohnson

@archmoj archmoj added the bug something broken label Nov 13, 2018
@etpinard
Copy link
Contributor

etpinard commented Nov 13, 2018

Yeah, that will fix the bug. I suspect users will notice some performance deterioration after on graphs with >256 colors, but I'm ok with showing the correct colors slowly as opposed to the wrong colors fast - until we figure out a way to use WebGL textures for graphs with more than 255 colors (tracked in gl-vis/regl-scatter2d#14)

@archmoj would you mind adding a mock to 🔒 this bug down? Grabbing the data/layout from https://codepen.io/alexcjohnson/pen/JeXOGX?editors=1010 should suffice.

package.json Outdated
@@ -103,7 +103,7 @@
"regl": "^1.3.7",
"regl-error2d": "^2.0.5",
"regl-line2d": "^3.0.12",
"regl-scatter2d": "^3.0.6",
"regl-scatter2d": "git://github.com/a-vis/regl-scatter2d.git#0515a47c5468363d19654fcb9383d7135c93c8ad",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

So this brings the max number of colors in a WebGL texture to 256x256=65536, and falls back to plain buffer beyond that? That's amazing! I gotta be honest, I don't fully understand your split8Uint dance, maybe you could explain it to me in person before or after the meeting tomorrow?

Now, looks like your latest commit made several image test cases fail, do you know why that is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I would fix this today. Don't worry about the failed image test for the moment. There are a bunch of commits which are not yet pushed to gl-vis.

@archmoj
Copy link
Contributor Author

archmoj commented Nov 14, 2018

@etpinard thanks for the comment. In respect to what you mentioned I revised other parts of regl-scatter2d which would allow using 65535 color palette.

@etpinard
Copy link
Contributor

@archmoj looks like you got the tests to ✅

Now, could you submit a PR to regl-scatter2d and ping @dy to see what he thinks?

@archmoj
Copy link
Contributor Author

archmoj commented Nov 15, 2018

@etpinard I would say we are pretty close to fix this. But there is still one thing remained that will show up in the coming tests.

@archmoj
Copy link
Contributor Author

archmoj commented Dec 12, 2018

Closed in regard with #3328. But I prefer not to delete the branch too for further work...

@archmoj archmoj closed this Dec 12, 2018
@etpinard etpinard deleted the issue-3232 branch December 17, 2018 17:33
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