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

Implement fast zsmooth option for image trace #5354

Merged
merged 14 commits into from
Jan 7, 2021
Merged

Conversation

almarklein
Copy link
Contributor

@almarklein almarklein commented Dec 18, 2020

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 is auto by default, the behavior of which is UA dependent. This is probably linear interpolation for most/all browsers, but we could consider using auto instead of linear.


<html>
<script src='../dist/plotly.js'></script>
<body>

<div id='myDiv'></div>

<script>
var source = "data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAADIAAAAlCAYAAAAA7LqSAAADmklEQVR4nO2XUWhTVxjHfyfdmKMN1oelK9QSLUVY2RWHrEKtLw66KQX30Ifpg0qxz31wb8O0UNjL1I3h09iDAy000D0Z0DJhEdslBXN7SwZBJmmbErKwNuU2UXDes4d4r700bib33KflD+Ge8+We78sv3/fdcy401VRTTTXVVFMIvxwnEhG5c97fP+lbLPARZHz8qBwY+AAA0zTp69MA/4B8cTo+flRq2n4MY42dMMFgEICRkZ+Uxw2odgigafudq2majn3nWLV8ATGMNdf44cPfd9lVyxeQwcF3MIw1gsEgY2OLzM9fYmDgPebnLzlQquVLj1y//pFcWan+R1eujLm+KxYTHDr0o/K4b6l2GI2efPnY3eTUqSMUiwmgBQDD+ANNC6sOCSgurUxmVAKUSpfp6tpHLJYiFksBL4AXaFr45Vy9lGUkkxmVhpEll9ukra1q6+raB+D68d3d76oK6ZKSjGQyozIWS5HLbVIujwCQyz0jHL4LVIHsT3f3r0xNfS3/zV8jUlpaNsT6eoS2tq/Q9TnSaZOZmRAzMyFKpcvo+hz5fEE5jGcQOxt2GeXzBQC2t6fY3p5y3bu+HnFsqmE8gdh9AThlZGtr67kzDoU6CIU6nPnGxioXLnzD0NCXLC7+rQTGE4jd3MePJ9H1OQBu3PjW2SO2tp6TTvfQ2dnhWjc9PSvs++2rVyl7aun6XTY2Vl22dLqHw4erp97Ozg6WlnqIRqfFqzVqIMADiL3x7cyGrb1733bGds8ArvK6f/+HRkPXVMNHhWj0pMzlNkkmw46tvT1NofAUTXtfPnnyrKbvgwf3yImJpPIznufSOns2C8Dt22EKhafMzq6KfL7XKpfZ1cStrUEeP/YasbY8Hd6KxS/kvXvfkc3eQdO+Z3j4kTh27BMrELCoVCpYlvUqkBBIKWltrb5cLSz8ojQrnpzZEOHwaYaHHwkAISSVSgWAlpYWDCMZMIxkwLIshBAIoXxTBzyA3Lr1p8xm7zA4+DnnzoUEwNWrnzkpEEKQSi04/peXFwMA5XKZoSGT8+c/tHZ7bVwNgcTjJQfixIl2AXDt2qeWlILe3iKAC8LW0lIicOZMdXzggNrDY90g8XhJPnjwswsCqiVlmn8hhETXf3ut38nJhC9vpXU5fR2ErWy2ws2by//pc2WlXE/YN9Ibg9gQ4fDpmhCgvlx8UTxekvF4qeYjJxLptyYmPq6reS9e7Kt7zf9C/wDLc4opp/2WUgAAAABJRU5ErkJggg==";

var data = [
    {"type": "image", "source": source, "xaxis": "x1", yaxis: "y1", "interpolate": "nearest"},
    {"type": "image", "source": source, "xaxis": "x2", yaxis: "y2", "interpolate": "linear"},
];

var layout = {
    "grid": {"rows": 1, "columns": 2, "pattern": "independent"},
    "width": 600, "height": 300,
    "margin": {"t": 35, "l": 35, "b": 35, "r": 35}
};

Plotly.newPlot('myDiv', data, layout, {scrollZoom: true});
</script>

</body>
</html>

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

Thanks, @almarklein for adding support for interpolation in the image trace.

At this point, you took care of one of the 2 rendering modes of image:

  • the fast one that simply displays an image element
  • legacy one (used for static image export) that draws a bunch of SVG rect in place of pixels

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.

cc @alexcjohnson @archmoj

@alexcjohnson
Copy link
Collaborator

This seems awfully similar to zsmooth='fast' for heatmaps:

zsmooth: {
valType: 'enumerated',
values: ['fast', 'best', false],
dflt: false,
role: 'style',
editType: 'calc',
description: [
'Picks a smoothing algorithm use to smooth `z` data.'
].join(' ')
},

would it be too awkward to use the same attribute name & values?

Also, what happens if fastImage is false? In that case it looks like we're filling rectangles that are larger than 1px with constant color. Doing manual bilinear interpolation in that case is what zsmooth='best' does for heatmaps.

@almarklein
Copy link
Contributor Author

would it be too awkward to use the same attribute name & values?

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:

  • make this property specific to image with source set (not good for consistency).
  • implement smoothing in the legacy rendering approach.

@alexcjohnson
Copy link
Collaborator

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.

@archmoj archmoj added the community community contribution label Dec 18, 2020
src/traces/image/plot.js Outdated Show resolved Hide resolved
@archmoj archmoj changed the title Add Image.interpolate property Implement fast zsmooth option for image trace Dec 21, 2020
@archmoj
Copy link
Contributor

archmoj commented Jan 4, 2021

@almarklein Happy New Year!
Are you still interested to finish this feature?

Co-authored-by: Mojtaba Samimi <33888540+archmoj@users.noreply.github.com>
@archmoj
Copy link
Contributor

archmoj commented Jan 4, 2021

The syntax test is failing due to new year headers.
When possible, please sync your fork then merge upstream/master into your branch.
Here you could find useful info https://digitaldrummerj.me/git-sync-fork-to-master/

@almarklein
Copy link
Contributor Author

@almarklein Happy New Year!
Are you still interested to finish this feature?

Happy 2021! Yeah I'll have at least another stab at it this week...

almarklein and others added 4 commits January 5, 2021 09:46
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>
Comment on lines 54 to 63
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(' ')
Copy link
Contributor Author

@almarklein almarklein Jan 5, 2021

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.

Copy link
Collaborator

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.

@archmoj
Copy link
Contributor

archmoj commented Jan 5, 2021

Thanks @almarklein for completing the PR.
Here are some minor syntax issues reported by the test-syntax.
Screenshot from 2021-01-05 09-34-38

@archmoj archmoj added this to the 1.59.0 milestone Jan 5, 2021
@almarklein
Copy link
Contributor Author

Here are some minor syntax issues reported by the test-syntax.

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

@archmoj
Copy link
Contributor

archmoj commented Jan 7, 2021

image_source_axis_reverse is broken.
Could you please investigate why?

@almarklein
Copy link
Contributor Author

It should be fixed now.

@archmoj
Copy link
Contributor

archmoj commented Jan 7, 2021

Nicely done.
💃
I'll add an image test in a follow up PR.

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

Successfully merging this pull request may close these issues.

Rendering Image traces with bilinear interpolation
4 participants