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

Make terrain in fall maps look more fall-like #66

Merged
merged 14 commits into from Jul 29, 2022

Conversation

cooljeanius
Copy link
Collaborator

Closes #63
(hopefully; let's see if the action works...)

@cooljeanius
Copy link
Collaborator Author

Darn @macabeus it didn't work... help?

Add to the list of stuff that shouldn't go on the server, as per this wiki page:
https://wiki.wesnoth.org/IGNFileFormat
(apparently if you manually specify stuff here, you also have to re-list the defaults)
@cooljeanius
Copy link
Collaborator Author

@nemaara did you want me to try to fix the map diff thing so it runs on PRs in this repo properly, or should I just run the tool manually and then hand-upload the diff images, or should I just merge without doing either of those?

@macabeus
Copy link

@cooljeanius I think the easiest way to fix it is editing the GitHub Action to clone wesnoth repository (it won't need to clone the whole history, but only the latest commit) and run it from there.

The issue is because it depends on the images and .cfg files that aren't present here.

This approach isn't pretty, but I don't have a better idea now. I didn't make map-diff planning to run on the repositories.

@nemaara
Copy link
Owner

nemaara commented May 31, 2022

@nemaara did you want me to try to fix the map diff thing so it runs on PRs in this repo properly, or should I just run the tool manually and then hand-upload the diff images, or should I just merge without doing either of those?

Doesn't matter to me, as long as nothing breaks in the release version.

@cooljeanius
Copy link
Collaborator Author

@cooljeanius I think the easiest way to fix it is editing the GitHub Action to clone wesnoth repository (it won't need to clone the whole history, but only the latest commit) and run it from there.

Hm I'm kinda bad at git; can you remind me how to do that? Also, besides just restricting it to the latest commit, can we also restrict it to just the directories we need, instead of getting the whole repository?

@macabeus
Copy link

macabeus commented Jun 1, 2022

@cooljeanius Command to clone only the latest commit:

git clone --depth 1 git@github.com:wesnoth/wesnoth.git

About cloning just a directory, it can be done: https://stackoverflow.com/a/52269934/3440266
I never did it, but it seems to work.
You would need to clone the folders ./data/core, ./images, and ./utils/wesnoth-map-diff (I hope not missing to mention a folder).

@cooljeanius
Copy link
Collaborator Author

cooljeanius commented Jun 2, 2022

@cooljeanius Command to clone only the latest commit:

git clone --depth 1 git@github.com:wesnoth/wesnoth.git

About cloning just a directory, it can be done: https://stackoverflow.com/a/52269934/3440266 I never did it, but it seems to work. You would need to clone the folders ./data/core, ./images, and ./utils/wesnoth-map-diff (I hope not missing to mention a folder).

hm ok I'm trying this, and while the commands don't seem to error out on me or anything, they also don't result in anything getting actually checked out for me either...

@cooljeanius
Copy link
Collaborator Author

OK I think the latest results exposed a bug in the wesnoth-map-diff tool:

/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/tilemap.js:59
        if (left[y][x].baseCode !== right[y][x].baseCode
                                                ^
TypeError: Cannot read properties of undefined (reading 'baseCode')
    at /home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/tilemap.js:59:49
    at walkthrough (/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/tilemap.js:41:9)
    at Object.diff (/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/tilemap.js:58:5)
    at paintTilemap (/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/paint.js:79:21)
    at Jimp.<anonymous> (/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/build/paint.js:84:9)
    at Timeout._onTimeout (/home/runner/work/A_New_Order/A_New_Order/utils/wesnoth-map-diff/node_modules/@jimp/core/dist/index.js:264:25)
    at listOnTimeout (node:internal/timers:559:17)
    at processTimers (node:internal/timers:502:7)
Error: Process completed with exit code 1.

(discussed on Discord, but I forget how far back that was, so I'm putting it here, too)

@macabeus
Copy link

macabeus commented Jun 6, 2022

@cooljeanius It's happening because there are maps with different dimensions.
wesnoth-map-diff won't work in this case.

We can add an if to skip the diff in this case (or update the code to support it)

@cooljeanius
Copy link
Collaborator Author

@cooljeanius It's happening because there are maps with different dimensions. wesnoth-map-diff won't work in this case.

We can add an if to skip the diff in this case (or update the code to support it)

I'm pretty sure I kept the dimensions the same in all of them; I thought it was just due to the

border_size=1
usage=map

lines... actually, maybe if I go and remove all of those manually first?

@macabeus
Copy link

macabeus commented Jun 6, 2022

Well... then I'm not sure what is happening. Maybe it failed to read the correct file?

We would need to debug to spot the issue. Firstly, I would simplify, opening a PR with just one map, then add some console.log to make a sanity check (print the filename, the file content, etc).

cooljeanius added a commit to cooljeanius/A_New_Order-1 that referenced this pull request Jun 7, 2022
merging this first in the hopes that it will get the diffs in PR nemaara#66 to display correctly; it's ok if the diff action fails for this one, because there shouldn't be any functional changes, just formatting
[ci skip]
@github-actions
Copy link

github-actions bot commented Jun 7, 2022

maps/Barnon.map


maps/Breaking_The_Circle.map


maps/BurnedCastle.map


maps/Crossroads.map


maps/GaeltinHyer.map


maps/OutlawBase.map


@cooljeanius
Copy link
Collaborator Author

eyyy it worked! @nemaara check it out!

@cooljeanius cooljeanius requested a review from nemaara June 7, 2022 03:59
@cooljeanius
Copy link
Collaborator Author

oops looking at the diff output made me realize I missed a few tiles; let me fix that...

@cooljeanius
Copy link
Collaborator Author

oh cool so it looks like it edits the existing comment with the new diffs instead of posting a new comment; that's convenient

a few more fall changes
@macabeus
Copy link

macabeus commented Jun 7, 2022

Amazing!! It's working!!! 🎉
Great job!

image

@cooljeanius
Copy link
Collaborator Author

Amazing!! It's working!!! 🎉 Great job!

One thing I do find kinda confusing, though, is how in normal diff views, the old content is on the left and the new content is on the right, but here it's switched so the new content is on the left and the old content is on the right...

@macabeus
Copy link

macabeus commented Jun 14, 2022

@cooljeanius On map-diff.yml, try replacing

node ./build/index.js "./$map_filename" "../../$map_path" "./$diff_image"

with

node ./build/index.js "../../$map_path" "./$map_filename" "./$diff_image"

@cooljeanius
Copy link
Collaborator Author

@cooljeanius On map-diff.yml, try replacing

node ./build/index.js "./$map_filename" "../../$map_path" "./$diff_image"

with

node ./build/index.js "../../$map_path" "./$map_filename" "./$diff_image"

ok that seems to have worked; thanks! @nemaara check it out!

@nemaara nemaara merged commit eda386e into nemaara:master Jul 29, 2022
@cooljeanius cooljeanius deleted the fall-terrain branch July 29, 2022 19:06
cooljeanius added a commit to cooljeanius/A_New_Order-1 that referenced this pull request Jul 29, 2022
this should get all the ones that I didn't touch in the fall terrain PR (nemaara#66)
ought to be just whitespace changes with no functional differences
[ci skip]
cooljeanius added a commit that referenced this pull request Jul 30, 2022
* Update _server.ign

Add to the list of stuff that shouldn't go on the server, as per this wiki page:
https://wiki.wesnoth.org/IGNFileFormat

* open and re-save all remaining maps w/editor

this should get all the ones that I didn't touch in the fall terrain PR (#66)
ought to be just whitespace changes with no functional differences
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.

Use autumn terrain when applicable
3 participants