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

Use CSS to support fast image rendering on linear axes #5307

Merged
merged 14 commits into from Nov 30, 2020

Conversation

almarklein
Copy link
Contributor

@almarklein almarklein commented Nov 27, 2020

Closes #5305

  • A mock based on existing image_axis_reverse is 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.
  • Styling is applied to flip the image as needed.
  • A test is added to ensure that the hover tooltip displays the correct info.

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

@almarklein
Copy link
Contributor Author

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.

@antoinerg
Copy link
Contributor

antoinerg commented Nov 27, 2020

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 😸

src/traces/image/plot.js Outdated Show resolved Hide resolved
src/traces/image/plot.js Outdated Show resolved Hide resolved
@antoinerg
Copy link
Contributor

Nice work @almarklein!

You can now 🔪 these lines since they don't apply anymore 🎉

['xaxis.range', [50, 0]],
['yaxis.range', [0, 50]]

@antoinerg antoinerg self-assigned this Nov 27, 2020
src/traces/image/plot.js Outdated Show resolved Hide resolved
src/traces/image/plot.js Outdated Show resolved Hide resolved
@almarklein
Copy link
Contributor Author

almarklein commented Nov 30, 2020

The image comparison tests also fails: not ok 162 image_opacity should be pixel perfect. Any idea why this mock would somehow be affected?

Could you please keep (expand) the old test with autorange and add new tests with defined ranges?

Will do.

src/traces/image/plot.js Outdated Show resolved Hide resolved
@archmoj
Copy link
Contributor

archmoj commented Nov 30, 2020

The image comparison tests also fails: not ok 162 image_opacity should be pixel perfect. Any idea why this mock would somehow be affected?

The transparency is applied by style in that case.
Please pick or apply these changes so that image_opacity renders with transparency.

@archmoj
Copy link
Contributor

archmoj commented Nov 30, 2020

The image comparison tests also fails: not ok 162 image_opacity should be pixel perfect. Any idea why this mock would somehow be affected?

The transparency is applied by style in that case.
Please pick or apply these changes so that image_opacity renders with transparency.

Here is the commit: c417e22

@archmoj archmoj changed the title Use CSS to support fast image rendering on any linear axes Use CSS to support fast image rendering on linear axes Nov 30, 2020
src/traces/image/plot.js Outdated Show resolved Hide resolved
@almarklein
Copy link
Contributor Author

almarklein commented Nov 30, 2020

Thanks! I added your commit to the PR.

It appears this block could be simplified to:

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: not ok 111 gl2d_subplots_anchor should be pixel perfect.

edit: nevermind, it looks like the image tests pass now.

@archmoj
Copy link
Contributor

archmoj commented Nov 30, 2020

@almarklein
I could simply add a commit on a dev branch to address all my above comments and generate the new baseline.
What do you think?

@archmoj
Copy link
Contributor

archmoj commented Nov 30, 2020

@almarklein
I could simply add a commit on a dev branch to address all my above comments and generate the new baseline.
What do you think?

Could you please pick 8a9380e and update PR description?

Comment on lines +688 to +691
// adjust considering css
if(test[0] === 'reversed') x = 512 - x;
if(test[1] !== 'reversed') y = 512 - y;
_hover(x, y);
Copy link
Contributor Author

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? ;)

Copy link
Contributor

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.

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, that bit is reassuring :)

@almarklein
Copy link
Contributor Author

Thanks for the help @archmoj !

@archmoj
Copy link
Contributor

archmoj commented Nov 30, 2020

Thanks for the help @archmoj !

My pleasure. You solved the tricky part 🥇

LGTM.
@antoinerg would you please review this PR?

@archmoj archmoj added this to the v1.58.0 milestone Nov 30, 2020
@antoinerg
Copy link
Contributor

Beautiful work @almarklein and thank you for addressing all the comments! I think the code got much cleaner

💃 💃 💃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for fast image rendering on any linear axes
3 participants