Custom compare pages script

[tomaas-zeman]

RFC. Created as a reaction to stalled PR https://github.com/Heegu-sama/Homm3BG/pull/417 on the same topic for my own purpose

The main thing here is that you don't need to build the baselines, it downloads the latest build from here for comparison and supports regular and printable versions

If you find this useful in general, I can create a proper PR from it

[DarthGandalf]

I haven't benchmarked this version of the script.

In #417 I have a feature that it creates a single file with multiple pages, which makes it easier to update the picture on github, when some multi-page PR changes, instead of adding all the files one by one. WDYT?

[tomaas-zeman]

I haven't benchmarked this version of the script.

In #417 I have a feature that it creates a single file with multiple pages, which makes it easier to update the picture on github, when some multi-page PR changes, instead of adding all the files one by one. WDYT?

Having them separately has the advantage that you can zoom in one by one. One multi-page image would be more difficult to view

[qwrtln]

I haven't benchmarked this version of the script.

In #417 I have a feature that it creates a single file with multiple pages, which makes it easier to update the picture on github, when some multi-page PR changes, instead of adding all the files one by one. WDYT?

I prefer split pages, as it allows you to explain them separately like in #568.

[DarthGandalf]

You can still run the script several times with different params to make them separate :)

Отправлено из мобильного приложения Proton Mail Android

-------- Оригинальное Сообщение -------- 09.06.2024 11:35, qwrtln написал:

I haven't benchmarked this version of the script.

In #417 I have a feature that it creates a single file with multiple pages, which makes it easier to update the picture on github, when some multi-page PR changes, instead of adding all the files one by one. WDYT?

I prefer split pages, as it allows you to explain them separately like in #568.

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: @.***>

[qwrtln]

You can still run the script several times with different params to make them separate :) Отправлено из мобильного приложения Proton Mail Android

Adding a "feature" just to have to keep working around this doesn't seem appealing to me. Well, but I won't argue about this any more. Running 4 commands with one different parameter instead of 1 every once in a while won't kill any pandas in the rainforest.

[tomaas-zeman]

You can still run the script several times with different params to make them separate :) Отправлено из мобильного приложения Proton Mail Android … -------- Оригинальное Сообщение -------- 09.06.2024 11:35, qwrtln написал:

I haven't benchmarked this version of the script. > > In #417 I have a feature that it creates a single file with multiple pages, which makes it easier to update the picture on github, when some multi-page PR changes, instead of adding all the files one by one. WDYT? I prefer split pages, as it allows you to explain them separately like in #568. — Reply to this email directly, [view it on GitHub](#567 (comment)), or unsubscribe. You are receiving this because you commented.Message ID: @.***>

What about an option to merge current outputs to a single one? Like -s | --single-file.

I prefer running a single command at all times - everybody has his own workflow and single command that could do both feels like a good compromise

[DarthGandalf]

Well, but I won't argue about this any more.

That feature is just what I'm using (and I had some valid use cases of having non-consecutive ranges of pages, but comma-separated, qpdf supports that). I don't insist on implementing it. But if the new script is better than mine (for my workflow), I'd gladly switch, otherwise I can just keep using mine, no big deal :)

I'm fine with using my script instead of the one submitted in the repo, that's what I've been doing so far.

And I don't insist on anyone to switch to my workflow.

[tomaas-zeman]

Well, but I won't argue about this any more.

That feature is just what I'm using (and I had some valid use cases of having non-consecutive ranges of pages, but comma-separated, qpdf supports that). I don't insist on implementing it. But if the new script is better than mine (for my workflow), I'd gladly switch, otherwise I can just keep using mine, no big deal :)

I'm fine with using my script instead of the one submitted in the repo, that's what I've been doing so far.

And I don't insist on anyone to switch to my workflow.

I'm not pushing my script either but I can try to add your workflow to see if we can have a single script that supports everyone

[tomaas-zeman]

I added the possibility of advanced range. Not great not terrible :) https://github.com/Heegu-sama/Homm3BG/pull/567/commits/9e63697fc1ddc64402087ce62b1b77f18b70a3db

It's slightly slower (in this basic version) than the original

# old
→ time ./tools/compare_pages.sh ~/Downloads/main_en.pdf cs 11 17
29.73s user 0.51s system 219% cpu 13.781 total

# new
→ time ./tools/compare_pages.sh -l cs -r 11-13,15,19-21  
33.60s user 0.70s system 602% cpu 5.692 total

[qwrtln]

Can you update the README file with a brief explanation of the new functionalities? The script is much larger now and harder to understand.

[tomaas-zeman]

Can you update the README file with a brief explanation of the new functionalities? The script is much larger now and harder to understand.

@DarthGandalf @qwrtln So do we want to have this merged? Does it support all your workflows?

[qwrtln]

@DarthGandalf @qwrtln So do we want to have this merged? Does it support all your workflows?

I like the idea that it's a replacement, not a competing tool with its own tradeoffs. I'm hoping that if we discover any major issues, you'll be able to fix them?

[DarthGandalf]

Works for me. The remaining concern is that it clutters . directory. Could it put them somewhere else?

[tomaas-zeman]

Works for me. The remaining concern is that it clutters . directory. Could it put them somewhere else?

That was there even before but it clutters it less now as it only copies the final images, not the intermediate. I don't have a strong opinion on the output directory. You can always do simple git clean

→ git clean -id
Would remove the following items:
  cs-12.png  cs-13.png  cs-14.png
*** Commands ***
    1: clean                2: filter by pattern    3: select by numbers    4: ask each             5: quit                 6: help
What now>

if we discover any major issues, you'll be able to fix them?

Yes

[DarthGandalf]

I don't have a strong opinion on the output directory

@qwrtln do you?

[qwrtln]

I don't have a strong opinion on the output directory

@qwrtln do you?

What's the issue? That the images are created in the repo root?

[DarthGandalf]

Yeah, unlike in /tmp or some other place which I clear from time to time.

[qwrtln]

Works for me. The remaining concern is that it clutters . directory. Could it put them somewhere else?

You can always do simple git clean

I always do git clean -f 😆

[qwrtln]

Yeah, unlike in /tmp or some other place which I clear from time to time.

Well, I don't know. I prefer them in the repo root. /tmp is cluttered by other apps anyway. And in the repo it's easier to trip over them, which reminds you to clean up after yourself. In my case, it's git clean -f.

[DarthGandalf]

Could it be a subdir of it then?

[qwrtln]

Could it be a subdir of it then?

As long as it's not in .gitignore, sure.

[tomaas-zeman]

I updated the PR, it's now mergable

• images are now stored in screenshots dir
• README was updated and simplified in a way that params and examples now sit with the script which should be easier to maintain
• more examples and comments in the code to hopefully make it more readable

Feel free to give it one more spin on your side to double-check it works as expected