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

Oxipng #1685

Open
wants to merge 7 commits into
base: next
Choose a base branch
from
Open

Oxipng #1685

wants to merge 7 commits into from

Conversation

mtolk
Copy link
Contributor

@mtolk mtolk commented Dec 7, 2021

IMPORTANT: Please do not create a Pull Request adding a new feature without discussing it first.

The place to discuss new features is the forum: https://zola.discourse.group/
If you want to add a new feature, please open a thread there first in the feature requests section.

Sanity check:

  • [ x ] Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Code changes

(Delete or ignore this section for documentation changes)

  • [ x ] Are you doing the PR on the next branch?

If the change is a new feature or adding to/changing an existing one:

  • [ x ] Have you created/updated the relevant documentation page(s)?

Updated my oxipng branch with latest changes from next. new pull request of the addition of configurable png optimization to zola.

@Keats Keats force-pushed the next branch 3 times, most recently from 1e55db1 to 3155662 Compare December 9, 2021 21:11
@Keats
Copy link
Collaborator

Keats commented Jan 13, 2022

https://github.com/shssoichiro/oxipng for people other than me not knowing what it was. I'll have a look at this PR as soon as the next bugfix version is released

Copy link
Collaborator

@Keats Keats left a comment

Choose a reason for hiding this comment

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

I think the optimization should be run before copying the assets so it happens only once but it means it would overwrite the existing file but I'm not sure how to recognize that a file has already been processed other than keeping some state file or something.

@@ -1127,6 +1128,18 @@ impl Site {
})
.collect::<Result<()>>()
}

/*
when running serve we do not want png optimizations, because that's too slow.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should only ever ran once for a file, after that it should be stored in static like the rest of the image processing so it's ok to run it on serve

/// Whether to optimize png files
pub optimize_png: bool,
/// level to optimize png files if applicable
pub optimize_png_level: u8,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this field can be an Option<u8> and you remove the need for optimize_png

@mtolk
Copy link
Contributor Author

mtolk commented Feb 1, 2022

Hi @Keats,

I agree it would be nicer if a file would only have to proces a file once. But then we would need some way of keeping track of which files are already processed and which not.

I'm not sure if that would be worth the effort.
Depending on the chosen solution it would make the code more complex, maybe also make the experience more complex for users. The way you would keep track of which file is optimized and which not, would that have to be under sourcecontrol? Do the optimized files have to be under source control( so that in my case netlify publishes the optimized versions).

It opens a whole can of worms, maybe it would be better to use webp with png as a fallback.

@Keats
Copy link
Collaborator

Keats commented Feb 8, 2022

Yeah I'm not too sure what to do honestly... Re-processing the files everytime is not great and you probably want to at least see what they look like on zola serve.

The way you would keep track of which file is optimized and which not, would that have to be under sourcecontrol?

Probably. Some simple json file or whatever that just stores a list of transformation/input and their hashes to see if anything is needed. Code wise, it would be a bit more complicated but not much more so. I expect there will be other transformations that will be needing the same kind of check, I think?

@mtolk
Copy link
Contributor Author

mtolk commented Feb 24, 2022

From what i understand from your comment is that optimized png's, the assets, would have to be under source control as well.
That would a problem for some sites. I have a site that generates multiple sizes from 1 source image using resize_image.
I would like to have all these generated images under source control, just the source image.

So I'm not sure we should proceed with this. It seems to add more complexity than that it adds value. I dont think it's worth it in this form.

@Keats
Copy link
Collaborator

Keats commented Feb 25, 2022

I would like to have all these generated images under source control, just the source image.

Do you mean you want only the source one or also the generated ones?

@mtolk
Copy link
Contributor Author

mtolk commented Feb 25, 2022

in my case i have just one image version under source control. during zola build other versions(different sizes but also webp copies) are created.
this way i dont have to keep track of all the different version of the image. should i make a change to it.

should you put the generated under source control then you could end up with lots of obsolete images in git, that you would have manually remove from the static\processed_images folder

@mwcz
Copy link
Contributor

mwcz commented Feb 25, 2022

It would be really surprising to me if the generated images were under source control.

@Keats
Copy link
Collaborator

Keats commented Feb 28, 2022

It would be really surprising to me if the generated images were under source control.

It's already the cases with resized images

@mwcz
Copy link
Contributor

mwcz commented Feb 28, 2022

It would be really surprising to me if the generated images were under source control.

It's already the cases with resized images

Consider me surprised. 😅

@Keats
Copy link
Collaborator

Keats commented Feb 28, 2022

To be fair they are all put in the same folders so you can gitignore it if you want.

@mtolk
Copy link
Contributor Author

mtolk commented Mar 1, 2022

The folder for resized images is always in my gitignore file. But then any administration you keep about files being optimized becomes irrelevant, because these files are generated and, if configured, optimized on every build.

@Jieiku
Copy link
Contributor

Jieiku commented May 28, 2022

oxipng is fast! so happy I found this pull request so I could learn of this tool!

@ISSOtm
Copy link
Contributor

ISSOtm commented Sep 19, 2023

If I may, it seems more appropriate to me to store the images separately (e.g. in a /src-imgs/ directory), and manually move them to /static///content/.

This takes care of the "have the images been processed already?" question, while already integrating nicely with the existing workflow/model (if you modify the source image but forget to compress it, then it will simply not update in the render, which should immediately signal that something is wrong. This also lets the user pick the optimisation settings that work best for them. (I've opted for the most aggressive settings since I'm only doing this once, but that's really slow.)

Also, on my personal website, I have PNGs, but also JPEGs and SVGs, which need to be processed by other tools. 1 (Tools list sourced from Squoosh.)

The obvious downside is a lack of discoverability; this could be solved by adding a page to the docs, I think.

Footnotes

  1. Yeah, that script re-processes all images in the site; I have in mind to only process images that have been modified since a certain (known-good) commit instead, but I've lazed out and not written that functionality yet.

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.

5 participants