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
Use CSS to support fast image rendering on linear axes #5307
Conversation
|
Actually, it looks like the tooltip displays the correct image values already. I don't know how the picking is implemented, so I have no idea if this makes sense. |
It looks good to me too. The hover routine is more robust than I thought: it does get the right pixel information regardless |
|
Nice work @almarklein! You can now plotly.js/test/jasmine/tests/image_test.js Lines 427 to 428 in cdd836c
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please keep (expand) the old test with autorange and add new tests with defined ranges?
|
The image comparison tests also fails:
Will do. |
The transparency is applied by |
Here is the commit: c417e22 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It appears this block could be simplified to:
if(fastImage) {
var flipX = xa.range[1] < xa.range[0];
var flipY = ya.range[1] > ya.range[0];
if(flipX || flipY) {
// Flip the SVG image as needed (around the proper center location)
style += 'transform:' +
'translate(50%,50%)' +
'scale(' + (flipX ? -1 : 1) + ',' + (flipY ? -1 : 1) + ')' +
'translate(-50%,-50%);';
}
}|
Thanks! I added your commit to the PR.
Wow, it does ... I'm pretty sure this is one of the things I've tried. Anyway, less code is better! It looks like the tests for the image_opacity are fixed. But now: edit: nevermind, it looks like the image tests pass now. |
|
@almarklein |
Could you please pick 8a9380e and update PR description? |
| // adjust considering css | ||
| if(test[0] === 'reversed') x = 512 - x; | ||
| if(test[1] !== 'reversed') y = 512 - y; | ||
| _hover(x, y); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like I was misunderstanding the way _hover() worked. Frankly, I'm still a little confused. They are neither "scene coordinates" nor screen coordinates. We are not cheating the test here, are we? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. That's why I added x and y to the hovertemplate test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that bit is reassuring :)
|
Thanks for the help @archmoj ! |
My pleasure. You solved the tricky part LGTM. |
|
Beautiful work @almarklein and thank you for addressing all the comments! I think the code got much cleaner |

Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.

Closes #5305
image_axis_reverseis added. I converted the image to base64-encoded PNG. I also swapped the red and green channel to avoid confusion between the two similar mock cases.Trying this out:
Test script to test with local (dev) plotly: https://gist.github.com/almarklein/db9e1a2faec6890d1156e6ec546d7ecc
Via codepen (uses latest Plotly from CDN): https://codepen.io/almarklein/pen/XWjbJyQ
@emmanuelle @antoinerg
cc: @plotly/plotly_js