Is preset 4 deprecated or incorrect?

[RReverser]

Currently --help says:

Optimization levels:
    -o 0  =>  --zc 3 --nz                  (0 or 1 trials)
    -o 1  =>  --zc 9                       (1 trial, determined heuristically)
    -o 2  =>  --zc 9 --zs 0-3 -f 0,5       (8 trials)
    -o 3  =>  --zc 9 --zs 0-3 -f 0-5       (24 trials)
    -o 4  =>  --zc 9 --zs 0-3 -f 0-5 -a    (24 trials + 6 alpha trials)
    -o 5  =>  --zc 3-9 --zs 0-3 -f 0-5 -a  (96 trials + 6 alpha trials)
    -o 6  =>  --zc 1-9 --zs 0-3 -f 0-5 -a  (180 trials + 6 alpha trials)

However, actual code for preset 4 is equivalent to preset 3 (no alpha optimisation):

    fn apply_preset_3(mut self) -> Self {
        for i in 1..5 {
            self.filter.insert(i);
        }
        self
    }

    fn apply_preset_4(self) -> Self {
        self.apply_preset_3()
    }

I suppose this is the outcome of a decision not to perform alpha optimisations by default, made in #187.

Which is fine, but makes state of this preset fairly confusing. Should it be removed at this point?

Also, docs for higher opt levels should probably be updated as well to exclude -a?

[TPS]

Maybe for backwards compatibility for existing users (especially scripting & such)?

[RReverser]

Maybe, but given the planned major release, it might be worth finally removing it.

And, even before that, the docs likely weren't updated by accident.

[shssoichiro]

I suppose this is the outcome of a decision not to perform alpha optimisations by default, made in #187.

Maybe for backwards compatibility for existing users (especially scripting & such)?

Both of these assumptions are correct. It may make sense to remove it, but my concern is for CLI users who are more likely to not expect a config option to be removed. For this reason, I suggest if we remove level 4 and squash the levels to 0-5, the CLI should allow level 6 and set it to the same as level 5. (It might even make sense to allow any preset level as input, and clamp to a max of level 5? This is what GCC does if you pass e.g. -O9 although I'm mixed on whether this is the best approach.)

(And yes, missing the documentation was an oversight.)

[TPS]

For my personal scripting, I'd actually prefer -o 9 (or whatever -max option) that would obviate the need to keep updating my code whenever oxipng is updated.

For folks involved in more heavy-duty applications, I can't imagine they'd have time/inclination to follow every possible breaking change here, especially as they'd be wrangling any № of packages' updates.

[RReverser]

For this reason, I suggest if we remove level 4 and squash the levels to 0-5, the CLI should allow level 6 and set it to the same as level 5.

FWIW I think if backward compatibility is a concern, then just adding a warning to -o 4 saying that it's deprecated is equally fine. Although on a major release it would probably be still reasonable to remove it rather than keep forever.

(It might even make sense to allow any preset level as input, and clamp to a max of level 5? This is what GCC does if you pass e.g. -O9 although I'm mixed on whether this is the best approach.)

For my personal scripting, I'd actually prefer -o 9 (or whatever -max option) that would obviate the need to keep updating my code whenever oxipng is updated.

Something like -o fast and -o best (and corresponding API-level methods) seem like a reasonable additions IMO that would allow to keep compatibility forever for those who don't care about anything in between.

[RReverser]

It might even make sense to allow any preset level as input, and clamp to a max of level 5? This is what GCC does if you pass e.g. -O9 although I'm mixed on whether this is the best approach.

Btw, it seems that this is already how it works:

oxipng/src/lib.rs

Lines 197 to 207 in 0a4bde1

	pub fn from_preset(level: u8) -> Options {
	let opts = Options::default();
	match level {
	0 => opts.apply_preset_0(),
	1 => opts.apply_preset_1(),
	2 => opts.apply_preset_2(),
	3 => opts.apply_preset_3(),
	4 => opts.apply_preset_4(),
	5 => opts.apply_preset_5(),
	_ => opts.apply_preset_6(),
	}

[RReverser]

Ah, nvm, that's how it works at API level but not in CLI due to validation.

[TPS]

I think this can be closed since #224 is merged?

[RReverser]

#224 adds warning, but if we are still to remove it in a major update, then it's useful to keep this open for tracking.

[andrews05]

Level 4 was reinstated in #457, so perhaps this could be closed now.

[RReverser]

Nice, thanks!