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

Introduces attribute source to image traces #5075

Merged
merged 26 commits into from
Aug 31, 2020
Merged

Introduces attribute source to image traces #5075

merged 26 commits into from
Aug 31, 2020

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Aug 14, 2020

which renders much faster!

Demo: https://codepen.io/antoinerg/full/wvGWgMY

TODO:

  • the plot step is now async. Fixed in e4b923b cc @alexcjohnson
  • download image in FF (Fixed in e4b923b)
  • test in all major browsers and fallback to old rendering method when necessary

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

archmoj commented Aug 14, 2020

@antoinerg could you possibly add the new mock image_labuda_droplets_source.json to mock_test.js after

'image_colormodel',

and test it after
figs['image_colormodel'] = require('@mocks/image_colormodel');

This is an extra step (until we could find a way to automate it) required to ensure the new mocks pass validation process.

@emmanuelle
Copy link
Contributor

Thank you @antoinerg ! Will there be a way to access the pixel value in a hovertemplate? If the answer is "no only the default hover is possible" it's ok, just wanted to know to adapt the python imshow function.

@antoinerg
Copy link
Contributor Author

@antoinerg could you possibly add the new mock image_labuda_droplets_source.json to mock_test.js after

'image_colormodel',

and test it after

figs['image_colormodel'] = require('@mocks/image_colormodel');

This is an extra step (until we could find a way to automate it) required to ensure the new mocks pass validation process.

@archmoj I am not familiar with mock_test.js. Is it new? What does it do?

@antoinerg
Copy link
Contributor Author

antoinerg commented Aug 14, 2020

Thank you @antoinerg ! Will there be a way to access the pixel value in a hovertemplate? If the answer is "no only the default hover is possible" it's ok, just wanted to know to adapt the python imshow function.

@emmanuelle: yes there is a way! See https://github.com/plotly/plotly.js/pull/5075/files#r470673042

@archmoj
Copy link
Contributor

archmoj commented Aug 14, 2020

@archmoj I am not familiar with mock_test.js. Is it new? What does it do?

It is something new added in #4762 a WIP to ensure our mocks pass Plotly.validate step and not to contain any extra info which could be confusing for our users.

@antoinerg
Copy link
Contributor Author

@archmoj I am not familiar with mock_test.js. Is it new? What does it do?

It is something new added in #4762 a WIP to ensure our mocks pass Plotly.validate step and not to contain any extra info which could be confusing for our users.

Done in e5bc429

cc @archmoj

var c;
for(i = 0; i < cd0.w; i++) {
var ipx0 = ipx(i); var ipx1 = ipx(i + 1);
if(ipx1 === ipx0 || isNaN(ipx1) || isNaN(ipx0)) continue;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need to test for NaNs?
Could we skip this process?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why we need to test for NaNs?
Could we skip this process?

I'm not sure at the moment.

However, this PR didn't change this logic: I simply moved it into a function. Could we maybe revisit this topic in a later PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

But they still be called in the case of non-fast-image!
So let's add some logic to skip these isNaN calls when source is present.

if(ipx1 === ipx0 || (
  !hasSource && (isNaN(ipx1) || isNaN(ipx0))
)) continue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not confident about this change. Checking for NaN might be necessary even when the image is defined via source if there are a logarithmic scale and negative values involved (ie. Math.log(-10) is NaN).

cc @archmoj

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright. Let's skip this for now.

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

antoinerg commented Aug 17, 2020

Using the following Codepen: https://codepen.io/antoinerg/full/wvGWgMY

I could test that Firefox, Edge, Chromium, Opera supports pixelated image-rendering on Windows, Mac, and Linux 🎉

Pixelated image rendering does NOT work in:

  • Safari (both desktop/mobile)
  • Both Chromium/Safari on iOS (so presumably iOS is a no-go regardless)
  • IE11

@antoinerg
Copy link
Contributor Author

Apparently IE11 has issues with onload handler for images and that's a showstopper. Investigating a workaround.

cc @nicolaskruchten @archmoj @alexcjohnson

@antoinerg
Copy link
Contributor Author

Lib.isIE -> Lib.isIE()

Fixed in f2caed8

@antoinerg
Copy link
Contributor Author

Apparently IE11 has issues with onload handler for images and that's a showstopper. Investigating a workaround.
cc @nicolaskruchten @archmoj @alexcjohnson

Safari has a similar issue to IE with onload but fortunately it seems restricted to <image> (an SVG element) and not <img> (an HTML element). I got this information from comments in this Stackoverflow thread.

Fixed with f2caed8 ! This also fixes npm run baseline so I could reintroduce image traces with source attribute into the test-image suite 🎉

@archmoj
Copy link
Contributor

archmoj commented Aug 18, 2020

Reacting source appears to be working fine | demo.
@nicolaskruchten are there any cases that you would like to be tested?

@nicolaskruchten
Copy link
Contributor

I'll play around with it a bit but I think @emmanuelle will want to recheck her open Python branch against the tip of this one :)

@antoinerg
Copy link
Contributor Author

@antoinerg
Could you provide source for image_labuda_droplets_source data? Is it a photo you have taken?

A friend of mine took it: https://blogs.royalsociety.org/publishing/oh-boy-quantum-droplets-win-the-2019-royal-society-publishing-photography-competition/

I'm pretty sure it would fall under fair usage but we should probably replace it with an image that is clearly in the public domain. I'm sure @emmanuelle would have good suggestions!

@archmoj I replaced the mock in b586a86

@antoinerg
Copy link
Contributor Author

In commit 3b75921 I slightly change the code to follow the pattern of if(hasZ) { } elseif(hasSource) {} throughout. It makes it simpler to read and reason about .

@archmoj
Copy link
Contributor

archmoj commented Aug 31, 2020

Great addition.
💃 💃 💃
🐢 🐢 🏇
🏇 🏇 🐢

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

Successfully merging this pull request may close these issues.

5 participants