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

Convert options #566

Merged
merged 4 commits into from
Apr 27, 2021
Merged

Convert options #566

merged 4 commits into from
Apr 27, 2021

Conversation

Alex-Jordan
Copy link
Contributor

This branch follows #557 and should perhaps be rebased once #557 is resolved.

This PR has a counterpart in webwork2: openwebwork/webwork2#1334

This allows the ImageMagick convert command to have options. The options I set for the default:
(1) increase the density that a PDF is carved up into before converting to another image
(2) increase the "quality" (decrease the compression) for building a PNG

The result is PNGs that look pretty good, and only take a little bit more space than the corresponding SVGs in my experiments.

Here is a test file, modified from the earlier PNG SVG comparison file. A line is commented out that sets options. It is commented out with the default options. I suggest seeing what happens when you revert to empty ([{},{}]).

DOCUMENT();

loadMacros("PGstandard.pl", "PGML.pl", "PGtikz.pl");

$a = random(-5, 5);

$tikz_code = <<END_TIKZ;
\tikzset{>={Stealth[scale=1.8]}}
\filldraw[draw=blue,fill=white,rounded corners=14pt,thick,use as bounding box] (-11,-11) rectangle (11,11);
\begin{scope}
	\clip[rounded corners=14pt] (-11,-11) rectangle (11,11);
	\draw[line width=0.2pt,color=lightgray,step=1] (-11,-11) grid (11,11);
\end{scope}
\huge
\draw[<->,thick] (-11,0) -- (11,0) node[above left,outer sep=2pt]{\(x\)};
\draw[<->,thick] (0,-11) -- (0,11) node[below right,outer sep=2pt]{\(y\)};
\foreach \x in {-10,-8,...,-2,2,4,...,10} \draw[thin] (\x,5pt) -- (\x,-5pt) node[below]{\(\x\)};
\foreach \y in {-10,-8,...,-2,2,4,...,10} \draw[thin] (5pt,\y) -- (-5pt,\y) node[left]{\(\y\)};
\draw[blue,line width=1.8pt,-{Stealth[scale=1.2]}] plot[domain=$a:11,samples=100,smooth]
(\x,{sqrt(\x-($a))});
\filldraw[red] ($a,0) circle[radius=4pt];
END_TIKZ

$graph_png = createTikZImage();
$graph_png->ext('png');
#$graph_png->convertOptions([{density => 300},{quality=>100}]);
$graph_png->tikzLibraries("arrows.meta");
$graph_png->tikzOptions("x=0.6cm,y=0.6cm");
$graph_png->tex($tikz_code);

$graph_svg = createTikZImage();
$graph_svg->tikzLibraries("arrows.meta");
$graph_svg->tikzOptions("x=0.6cm,y=0.6cm");
$graph_svg->tex($tikz_code);

BEGIN_PGML
[@ image(insertGraph($graph_png), width => 350, tex_size => 400) @]*
[@ image(insertGraph($graph_svg), width => 350, tex_size => 400) @]*
END_PGML

ENDDOCUMENT();

Note I am not wedded to the default options I set. These are just what PreTeXt settled on for a similar thing, so I started there.

@Alex-Jordan Alex-Jordan changed the base branch from master to PG-2.16 April 19, 2021 23:58
Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good, although maybe it would be more straightforward to have two options, say convertInputOptions and convertOutputOptions, instead of an array with two elements.

This is another thing that I had considered doing to improve png image generation. You got to it first though!

lib/TikZImage.pm Outdated
@@ -35,7 +35,8 @@ sub new {
tikzLibraries => '',
texPackages => {},
addToPreamble => '',
ext => 'png',
ext => 'svg',
svgMethod => 'pdf2svg',
Copy link
Member

Choose a reason for hiding this comment

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

I missed this before. The 'convertOptions' should be added here to ensure that they are defined for the object. You are perhaps assuming that this is always used via PGtikz.pl, but that may not be the case. This package can be used directly, in which case the defaults will not be set.

@Alex-Jordan
Copy link
Contributor Author

convertInputOptions and convertOutputOptions

What's your preference?

$convertInputOptions = {...}
$convertOutputOptions = {...}
$convertOptions = {input => {...}, output =>{...}}
$convertOptions = [{...}, {...}]   # presently

I could go any way. Maybe it should even be more like:

$tikzProcessingOptions{convert} = ...

to generalize better if other tools need options.

@drgrice1
Copy link
Member

I will let you decide which is best, but I think I prefer input/output hash keys. Also, incorporating tikz into the option name would probably be good. In the TikZImage package the name of the option should be just convertOptions (or perhaps pngConvertOptions?) and not include tikz, but in localOverrides the name of the option should probably include tikz in some way.

@drgrice1
Copy link
Member

drgrice1 commented Apr 20, 2021

It also occurred to me that this would be a valid option that a problem author might want to change. So perhaps it would be a good idea to document it in the POD of PGtikz.pl. For example, a problem author could use

$graph = createTikZImage();
$graph->ext('png');
$graph->convertOptions([{density => 300}, {quality => 100, resize => "500x500"}]);
$image->BEGIN_TIKZ
...
END_TIKZ

This works with your current code, and of course would need to be modified for whatever format you settle on for the option.

@Alex-Jordan
Copy link
Contributor Author

I changed the structure to be like {input => {},output => {}} and renamed the global default $tikzConvertOptions. Added to the POD. Initialized the options hash as you mentioned. Note both PRs have an update now.

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

Thanks for adding the changes I suggested. Looks good.

@taniwallach
Copy link
Member

@Alex-Jordan #557 was merged, and this now has merge conflicts... so apparently need a bit of work before we can test and merge it.

@Alex-Jordan
Copy link
Contributor Author

I rebased and addressed the conflict, then force pushed. (Also rebased and force pushed the corresponding webwork2 PR.)

Copy link
Member

@drgrice1 drgrice1 left a comment

Choose a reason for hiding this comment

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

This looks good.

ext => 'svg',
svgMethod => 'pdf2svg',
convertOptions => {input => {},output => {}},
imageName => ''
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for fixing my indentation error!

@drgrice1
Copy link
Member

Attached is a test file. The first image
comparison.zip
is a png image generated with the default settings. The second image is a png image generated with $graph_png2->convertOptions({ input => {density => 300}, output => {quality => 100, resize => "500x500"} });. The third image is an svg image (dvisvgm was used but I doubt you could tell the difference if pdf2svg were used in this case).

@drgrice1 drgrice1 requested a review from pstaabp April 22, 2021 01:09
@Alex-Jordan
Copy link
Contributor Author

The SVG in that example is 32KB (at least with my system that is using pdf2svg).

The default PNG (with density 300, quality 100) is 35KB.

The other PNG that is rescaled to 500x500 is 102KB. I find it interesting that it does not look as good (imho) as the 35KB one. It is fuzzier, some side effect of the rescaling.

If I use $graph_png2->convertOptions({ input => {}, output => {} }); (so, reverting to convert's defaults) I get an 8KB file. It is a bit fuzzier than the 102KB file.

@drgrice1
Copy link
Member

I saw the same thing. The 500x500 image does seem fuzzier. It may be possible to fiddle with the settings and get it a bit better, but I have little confidence in imagemagick's convert to do a good job with this. That is why I highly recommend the svg image when possible.

@drgrice1
Copy link
Member

Although, I also noted that the second (500x500) png image looks less fuzzy than the first (with your convert defaults) in that example when enlarged.

@drgrice1
Copy link
Member

If you increase the input density to 500 the image quality improves for the second png, and the file size is less.

@Alex-Jordan
Copy link
Contributor Author

Although, I also noted that the second (500x500) png image looks less fuzzy than the first (with your convert defaults) in that example when enlarged.

I wonder if we have different versions of ImageMagick and/or latex and that we are seeing different things. For me, among the pngs, the density 300 quality 100 is the clear winner. I can't even distinguish it from the svg without zooming. Here are five versions I'm getting: http://spot.pcc.edu/~ajordan/tikzimages/

@drgrice1
Copy link
Member

The files generated on your system are different than those generated on mine. The file sizes are different. However, the file sizes are close, and the visual quality is similar. I think I confused the case with the convert options in defaults.config with the override to no defaults before. I did them at different times in the same place in the problem. Looking again I see something more like you are seeing. One thing to note though is that the density 300 - output 100 options is a 1562x1562 pixel image. So it is really being shrunk to fit the 350x350 output size in the problem. When opened in the image dialog it immediately fills the entire window (since the dialog opens to the natural size limited by the window size). While the resized image opens in the dialog to a more reasonable 500x500 size.

I noticed now that if you use $graph_png->convertOptions({ input => {density => 300}, output => {quality => 100, scale => "500x500"} }); it does a better job of resizing. I can't tell the difference when the images are displayed at the 350x350 size. When enlarged it gets a little fuzzy, but is not bad.

Imagemagick's convert has a lot more options to play with, and many of them are not documented. I remember finding some pdf specific options online at one point that can give lossless conversion, but I can't find them anymore.

In any case, for usage in the browser svg wins hands down in my opinion.

@drgrice1
Copy link
Member

I am going to go ahead and merge this. It passes my tests. It has been sitting here for a while now, so I think there has been plenty of time to review.

@drgrice1 drgrice1 merged commit 04189e1 into openwebwork:PG-2.16 Apr 27, 2021
drgrice1 added a commit that referenced this pull request Apr 27, 2021
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.

3 participants