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

feat: enable ppi setting on png cli export #8854

Merged
merged 1 commit into from
Apr 27, 2023
Merged

Conversation

davidanthoff
Copy link
Contributor

In theory this might enable saving of PNG files at higher ppi resolutions, for example create a high quality 300 ppi PNG file that still has the same height and width in physical measures. The existing scale argument does not allow that.

I mentioned this previously at vega/vega#2269. If this PR here works I would add the same thing to vega-cli.

I don't have a setup to test any of this :) So this is a blind PR. Sorry, I know this is not ideal...

Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thank you. Instead of ppi, can we use scale as it's the actual argument Vega uses and therefore avoids confusion?

@davidanthoff
Copy link
Contributor Author

I think they are different things?

In my mind scale actually changes the size of a figure in physical units, say if it had dimension 96x96 beforehand, which would be 1inch x 1inch, then for a scale of 2 you get a 2inch x 2inch figure.

But ppi would not change the dimensions in inch of the figure, it just controls how many pixels per inch are used for the rasterization process.

@domoritz
Copy link
Member

domoritz commented Apr 21, 2023

But in the end you set the scale of toCanvas so it's weird if we call the argument something else from what we already use in Vega.

I don't know whether ping even has ppi. I thought it just has a resolution. See also https://graphicdesign.stackexchange.com/questions/107888/check-dpi-of-png-file. Where does 96 come from btw?

@davidanthoff
Copy link
Contributor Author

Where does 96 come from btw?

Ah, I just did a bit more experimenting, and that was wrong, that should be 72 :) As far as I can tell, if I specify a width in vega or vega-lite, that is automatically interpreted as having a unit of pt from here.

Here is how I came to that conclusion: when I save a vega-lite spec that has height of 72 * 3 and width of 72 *3, and I use autosize fit with padding for contains, and I export that as either a pdf or svg, I get figures that have exactly a 3x3 inch dimension recorded in those files.

When I export the same figure with a scale of 2, then in both cases I get figures that are 6x6 inches. So the current interpretation of the scale command line argument is to change the overall size of the figure for pdf and svg.

For PNG files in general the situation is a bit trickier: by default node-canvas doesn't embed any physical size information in a PNG file. It will export a PNG file that doesn't have any physical size information, it will just be a PNG file with x times y pixels, but no additional information how much space in a physical unit a given pixel should take say on a monitor or when printed. This PR retains that behavior if the ppi option is not specified: if I have a vega-lite spec with 300x300 width and height, it will produce a PNG that has those pixels, end of story. If I specify a scale of say 2 at the command line, it will produce a PNG with 600x600 pixels, also retaining the current behavior.

But, PNG files can also contain information how big each pixel should be in physical units. In this PR that info is embedded when one specifies the ppi option (technically one now creates a PNG file with info for PixelUnits as meters and then info PixelsPerUnitX and PixelsPerUnitY, that is how that info is stored in PNG files. All viewers seem to convert that into ppi when displaying). In my mind, when we do embed this kind of info in a PNG, I think we want to make the behavior of the command line tools mimic the export behavior for PDF and SVG, right? That is what this PR here does: if you export a spec that has width 72 *3 and height 72 * 3 with say --ppi 300 then you get a PNG file that has dimension 3x3 inch (exactly like the PDF or SVG export), and it uses 300 pixels per inch, so the resolution of the PNG file is 900x900. If you export the same spec with --ppi 200 and -s 2 you get a figure that is 6x6 inches, and the resolution of the PNG file is 1200x1200.

But in the end you set the scale of toCanvas so it's weird if we call the argument something else from what we already use in Vega.

Yes, I'm utilizing the scale parameter of toCanvas to create PNG files that have the correct pixels and resolution that one would expect depending on what arguments one specified for -s and --ppi at the command line, I don't really see the problem there?

I think the core idea of this PR is that the scale command line argument should resize the physical size of a figure consistently across all export file types, and then for a raster export format like PNG we need another command line argument to specify the ppi we want to use.

I don't know whether ping even has ppi.

https://jimpl.com/ is a convenient tool to extract that kind of information. PNG doesn't store ppi directly, it uses pixels per meter, but no piece of software that I found directly exposes that to users, they all seem to use ppi instead (including node-canvas).

@domoritz
Copy link
Member

Thanks for the insightful discussion. I think I'm more convinced now but I will need to test this before merging. Also, shouldn't Vega-cli have the same option?

@davidanthoff
Copy link
Contributor Author

I will need to test this before merging

Yes, that would be great! I did manage to run things on my system now, so this is no longer a completely untested PR. The way I tested this is that I created PNG files and then looked at them on that web page to see whether the info is correct.

Also, shouldn't Vega-cli have the same option?

Yes, PR I'll open a PR in a second.

@domoritz
Copy link
Member

Fantastic. Thank you. I'll wait for vega/vega#3727 to land so we are in sync between Vega and Vega-Lite.

@davidanthoff
Copy link
Contributor Author

@domoritz Any chance you could merge this, and then maybe even tag a new release with this and my other PR? I have some new stuff on the Julia side ready to go as soon as there are new versions released here. Thanks :)

@domoritz domoritz merged commit d3a30bf into vega:main Apr 27, 2023
@domoritz
Copy link
Member

I would like to have #8857 in the next release.

@davidanthoff davidanthoff deleted the ppi branch April 27, 2023 19:54
@domoritz
Copy link
Member

@kanitw are you making a release with the changes you sent soon? If not, let's cut a release.

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

Successfully merging this pull request may close these issues.

2 participants