Add `wesnoth-map-diff` #6472

macabeus · 2022-01-30T02:36:57Z

This PR is a proposal to add a new command-line tool, wesnoth-map-diff.

It aims to be a CLI to produce an image comparing two tilemaps, used by a GitHub Action to comment on a PR automatically:

I'm writing it using JS because it's the language I'm most used to and be easy to develop this kind of tool.
To use it, you need to have Node.js. Then on your terminal:

> cd utils/wesnoth-map-diff
> npm i
> node src/index.js

Wedge009 · 2022-01-31T03:52:13Z

Good on you for giving this a try. If you're not finished, you can always switch the PR to a draft.

stevecotton · 2022-03-07T09:35:42Z

.github/workflows/map-diff.yml

+        for diff_image in ${{ steps.map-diff.outputs.DIFF_IMAGES }}
+        do
+          image_link=$(curl -X POST "https://api.imgur.com/3/upload" \
+            -F "image=@\"$diff_image\"" | jq ".data.link" -r)


How often will this upload images?

It uploads an image whenever a PR (with a .map file) is opened or updated.

stevecotton · 2022-03-07T09:36:54Z

.github/workflows/map-diff.yml

+      run: |
+        cd ./utils/wesnoth-map-diff
+
+        sudo apt-get install -y pngquant


There's already utils/woptipng.py

I didn't notice this tool before.

But anyway, utils/woptipng.py isn't standalone.
It depends on a set of different packages (Pillow, advdef, imagemagick/convert, and optipng).

I think it's simpler just to install a single package (pngquant) and run it on this action.
pngquant works well for this kind of image as far as I checked.

.github/workflows/map-diff.yml

cooljeanius · 2022-03-13T15:32:21Z

This seems really cool; how do I use it in my repositories for UMC campaigns that I maintain?

CelticMinstrel · 2022-03-13T15:45:20Z

The only thing I don't quite understand is… why upload the image to Imgur when GitHub itself supports image attachments on comments?

@cooljeanius It might be enough to just copy the workflow file to your repo (once this is merged).

Elvish-Hunter · 2022-03-13T18:11:05Z

Quick question: since most of our users are on Windows, how can they run this tool on Windows systems? IMHO, a text file which explains how to do so would be appreciated.

macabeus · 2022-03-13T19:12:23Z

@cooljeanius

This seems really cool; how do I use it in my repositories for UMC campaigns that I maintain?

There are two options:

run manually on your computer. You can follow the readme.
run using the github actions. It should work seamlessly just by copying and pasting.

@CelticMinstrel

why upload the image to Imgur when GitHub itself supports image attachments on comments?

Firstly, I tried to upload the image to GitHub. But it's harder since we need to have an authentication token.
Uploading to Imgur is easier, so I picked this solution.

@Elvish-Hunter

Quick question: since most of our users are on Windows, how can they run this tool on Windows systems? IMHO, a text file which explains how to do so would be appreciated.

There is the readme explaining it. But I can try to explain in more detail how to use it on Windows.

Elvish-Hunter · 2022-03-13T21:10:27Z

There is the readme explaining it. But I can try to explain in more detail how to use it on Windows.

@macabeus Yes, I think that some detailed instructions for our Windows users would be really useful.

macabeus · 2022-03-13T23:26:01Z

@Elvish-Hunter I just added more detail on the readme. Please, let me know if it's better now

soliton- · 2022-03-13T16:12:04Z

.github/workflows/map-diff.yml

+        ## Get Maps
+        git fetch --depth=1 origin ${{ github.event.pull_request.base.sha }}
+
+        map_paths=($(git diff --name-only HEAD ${{ github.event.pull_request.base.sha }} | grep ".map$"))


I cannot reproduce the issue with git diff --name-only HEAD ${{ github.event.pull_request.base.sha }} '*.map' but I think strictly it should be git diff --name-only HEAD ${{ github.event.pull_request.base.sha }} -- '*.map'

If grep is really necessary then it should be grep '\.map$' since a dot matches any character.

To not use word splitting and unintentionally pathname expansion it'd be better to use mapfile here:
mapfile -t map_paths < <(git diff --name-only HEAD ${{ github.event.pull_request.base.sha }} -- '*.map')

You can reproduce the issue using my test repository:

> git clone git@github.com:macabeus/wesnoth.git > cd wesnoth > git checkout feat/add-wesnoth-map-diff-backup > git diff --name-only HEAD 514b8bca9cf -- '*.map'

I cannot reproduce it there either. For me git works as documented.

$ git diff --name-only HEAD 514b8bca9cf -- '*.map' data/test.map data/test2.map

Anyway it's not that big a deal to keep the grep as well.

.github/workflows/map-diff.yml

Pentarctagon · 2022-04-14T00:04:26Z

If all comments have been addressed, it sounds like this can be merged?

stevecotton · 2022-04-14T10:18:00Z

Do any of the reviewers know enough about the Node.js ecosystem to review this? I don't, @soliton- appears to be only reviewing the shell-script part, and the other commentators seem to be asking about how to run it rather than reviewing it.

soliton- · 2022-04-14T11:46:03Z

Yeah, I didn't and don't intend to look at the js part.

soliton- · 2022-04-26T08:30:55Z

Since it doesn't seem we'll get anyone to review the js part I would suggest to just merge this and see how it goes.

Pentarctagon · 2022-04-27T00:00:12Z

I'll merge tomorrow then, barring anything else coming up.

macabeus force-pushed the feat/add-wesnoth-map-diff branch 2 times, most recently from 00d1921 to b0fe192 Compare January 30, 2022 02:56

Wedge009 added the Editor Map/scenario editor issues. label Jan 31, 2022

macabeus marked this pull request as draft January 31, 2022 09:47

macabeus force-pushed the feat/add-wesnoth-map-diff branch 21 times, most recently from a0f8f61 to 10420e4 Compare March 6, 2022 21:18

stevecotton reviewed Mar 7, 2022

View reviewed changes

soliton- reviewed Mar 9, 2022

View reviewed changes

.github/workflows/map-diff.yml Outdated Show resolved Hide resolved

.github/workflows/map-diff.yml Outdated Show resolved Hide resolved

.github/workflows/map-diff.yml Outdated Show resolved Hide resolved

macabeus force-pushed the feat/add-wesnoth-map-diff branch from 10420e4 to 396b27f Compare March 12, 2022 21:54

macabeus force-pushed the feat/add-wesnoth-map-diff branch 6 times, most recently from 5b0430b to aeb9f83 Compare March 13, 2022 15:22

Add wesnoth-map-diff

16fbb5f

macabeus force-pushed the feat/add-wesnoth-map-diff branch from aeb9f83 to 16fbb5f Compare March 13, 2022 15:24

macabeus marked this pull request as ready for review March 13, 2022 15:25

wesnoth-map-diff: add more detail on readme

11af090

macabeus requested review from stevecotton and soliton- March 13, 2022 23:26

soliton- reviewed Mar 15, 2022

View reviewed changes

wesnoth-map-diff: improve github action code quality

322fc69

macabeus requested a review from soliton- March 17, 2022 00:43

macabeus mentioned this pull request Mar 20, 2022

Fix sidebar abilities leaking shrouded information #6471

Closed

Pentarctagon merged commit d108305 into wesnoth:master Apr 27, 2022

cooljeanius mentioned this pull request May 28, 2022

Use autumn terrain when applicable nemaara/A_New_Order#63

Closed

cooljeanius mentioned this pull request Jun 25, 2022

Make a "map diff" tool to visualizes changes between two version of a map #2323

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `wesnoth-map-diff` #6472

Add `wesnoth-map-diff` #6472

macabeus commented Jan 30, 2022 •

edited

Wedge009 commented Jan 31, 2022

stevecotton Mar 7, 2022

macabeus Mar 7, 2022 •

edited

stevecotton Mar 7, 2022

macabeus Mar 12, 2022 •

edited

cooljeanius commented Mar 13, 2022

CelticMinstrel commented Mar 13, 2022 •

edited

Elvish-Hunter commented Mar 13, 2022

macabeus commented Mar 13, 2022 •

edited

Elvish-Hunter commented Mar 13, 2022

macabeus commented Mar 13, 2022

soliton- Mar 13, 2022

macabeus Mar 16, 2022

soliton- Apr 14, 2022

Pentarctagon commented Apr 14, 2022

stevecotton commented Apr 14, 2022

soliton- commented Apr 14, 2022

soliton- commented Apr 26, 2022

Pentarctagon commented Apr 27, 2022

Add wesnoth-map-diff #6472

Add wesnoth-map-diff #6472

Conversation

macabeus commented Jan 30, 2022 • edited

Wedge009 commented Jan 31, 2022

stevecotton Mar 7, 2022

Choose a reason for hiding this comment

macabeus Mar 7, 2022 • edited

Choose a reason for hiding this comment

stevecotton Mar 7, 2022

Choose a reason for hiding this comment

macabeus Mar 12, 2022 • edited

Choose a reason for hiding this comment

cooljeanius commented Mar 13, 2022

CelticMinstrel commented Mar 13, 2022 • edited

Elvish-Hunter commented Mar 13, 2022

macabeus commented Mar 13, 2022 • edited

Elvish-Hunter commented Mar 13, 2022

macabeus commented Mar 13, 2022

soliton- Mar 13, 2022

Choose a reason for hiding this comment

macabeus Mar 16, 2022

Choose a reason for hiding this comment

soliton- Apr 14, 2022

Choose a reason for hiding this comment

Pentarctagon commented Apr 14, 2022

stevecotton commented Apr 14, 2022

soliton- commented Apr 14, 2022

soliton- commented Apr 26, 2022

Pentarctagon commented Apr 27, 2022

Add `wesnoth-map-diff` #6472

Add `wesnoth-map-diff` #6472

macabeus commented Jan 30, 2022 •

edited

macabeus Mar 7, 2022 •

edited

macabeus Mar 12, 2022 •

edited

CelticMinstrel commented Mar 13, 2022 •

edited

macabeus commented Mar 13, 2022 •

edited