-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Implement fast zsmooth option for image trace #5354
Conversation
Thanks, @almarklein for adding support for interpolation in the At this point, you took care of one of the 2 rendering modes of
I'm not sure what would be the best way to add support for interpolation in the legacy mode. But when it's done, it should be 🔒 down via an image test. |
This seems awfully similar to plotly.js/src/traces/heatmap/attributes.js Lines 80 to 89 in d45eef5
would it be too awkward to use the same attribute name & values? Also, what happens if |
No, +1 for consistency. This also solves the issue that we don't know the interpolation method that a browser will actually use. So I think either:
|
It's fine to implement this only in a subset of situations for now, as long as we won't have to change the existing behavior if and when we extend it to more situations, and as long as we document the limitations. |
@almarklein Happy New Year! |
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
The syntax test is failing due to new year headers. |
Happy 2021! Yeah I'll have at least another stab at it this week... |
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
src/traces/image/attributes.js
Outdated
zsmooth: { | ||
valType: 'enumerated', | ||
values: ['fast', false], | ||
dflt: false, | ||
role: 'info', | ||
editType: 'plot', | ||
description: [ | ||
'Picks a smoothing algorithm used to smooth `z` data.', | ||
'This only applies for image traces that use the `source` attribute.' | ||
].join(' ') |
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.
The heatmap trace has ['fast', 'best', false]
. I'm not sure what false
means there, but would it not make more sense to have 'fast' and 'best' here, which would resolve to pixelated and "auto", respectively?
We should also add a note that the result may be browser dependent.
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.
Ha, we really need better documentation of those options for heatmaps, but here's what they mean:
false
means no smoothing, each data point is a rectangle (or "brick") of constant color.'fast'
means we let the browser interpolate smoothly between one data value and the next (so yes, the result may be browser-dependent). This only works when all pixels are the same size, and it can cause problems if two neighboring points have very different value, as the browser will probably interpolate linearly in RGB space and intermediate values may not be present in the colorscale at all.'best'
means we smooth by calculating the color at each screen pixel independently, first with bilinear interpolation of data values, then we map the result onto the colorscale. This can be slow, but it handles nonuniform x or y spacing and ensures every interpolated value is in the colorscale.
So what you've implemented here - assuming the browser behaves as expected - corresponds to false
(pixelated) and 'fast'
(smoothed). 'best'
probably doesn't make sense for images unless there are important browsers that we can't get to behave correctly with 'fast'
. But we don't support nonuniform pixels in image traces, and there's no colorscale so the interpolation we would do manually is likely the same as the browser does, unless perhaps we wanted to support interpolating in HSL space.
Thanks @almarklein for completing the PR. |
Sorry about that. I'm on another machine and did not have Eslint et al. configured yet. I thought I could get away with it - apparently not :P |
|
It should be fixed now. |
Nicely done. |
Closes #5353
I confirmed the working using a simple html doc.
What's the best way to add a test for this? I'm not sure if an image-based test will be flaky?
The value of
image-rendering
, according to the MDN docs isauto
by default, the behavior of which is UA dependent. This is probably linear interpolation for most/all browsers, but we could consider usingauto
instead oflinear
.