Campaign screen final

[ottml]

Fixes https://github.com/Return-To-The-Roots/s25client/issues/568 Relates too #1591

[ottml]

[Spikeone]

Looking pretty good - although it's sad we don't have the original worldmap screen, I don't think we need that (for now).

[Spikeone]

Oh, and we still need some kind of "patch map" function where one is able to override parts of the original map file (to fix BBs maps) - since this is only required for the original roman campaign, it could be just giving an index and new hex value which changes the mapfile after initially loading it.

[ottml]

@Flamefire @Flow86 can you have a look at the pull request and make a review. Would be nice. Thanks.

[Spikeone]

@Flamefire shouldn't we put campaigns into the s25maps repo as well?

[Flamefire]

@Flamefire shouldn't we put campaigns into the s25maps repo as well?

I'd say it makes sense to have "our" campaigns here. At least the lua files. Otherwise we'd need another submodule and as LUA files are text they don't hurt to have in this repo. Only binary files like maps should be separate. At least that was the intention for the maps repo IIRC.

[Spikeone]

@Flamefire shouldn't we put campaigns into the s25maps repo as well?

I'd say it makes sense to have "our" campaigns here. At least the lua files. Otherwise we'd need another submodule and as LUA files are text they don't hurt to have in this repo. Only binary files like maps should be separate. At least that was the intention for the maps repo IIRC.

So, since I have a custom campaign which was made by a fan, we then init that one? Fine for me, was just thinking about easy PRs, even for the campaign maps

[Flamefire]

So, since I have a custom campaign which was made by a fan, we then init that one? Fine for me, was just thinking about easy PRs, even for the campaign maps

Makes sense I guess. No need to run the full CI suite for campaign PRs. On the other hand: We need a PR here for the submodule update then which results in the same. But with the (potential) maps it makes sense to pull it out. I guess we'll need a new repo for that. @Flow86 ?

However I wouldn't block the PR for that: Let's get this in first, moving the text files into a subproject can be done afterwards.

[Flamefire]

Please don't force-push during a review phase. It is difficult to find what you have changed, i.e. what parts of the review comments were adressed, after a force-push. You can commit with git commit --fixup=<original commit sha> to note that this previous commit should be fixed/changed and do that automatically after the review/before merge with git rebase --autosquash

[ottml]

Please don't force-push during a review phase. It is difficult to find what you have changed, i.e. what parts of the review comments were adressed, after a force-push. You can commit with git commit --fixup=<original commit sha> to note that this previous commit should be fixed/changed and do that automatically after the review/before merge with git rebase --autosquash

Thanks for the hint. I will do this from now on. I do not know that this is possible in such an easy way.

[Flamefire]

Thanks for the hint. I will do this from now on. I do not know that this is possible in such an easy way.

No worries, that seems to be something new to git as I learned about that only recently. Previously I did that manually with stuff like git commit -m 'f: Part of another commit title' and then do the fixups with an interactive rebase manually (git rebase -i)

And with that change from GitHub showing the diff of a force-push it actually got easier to even track those.

[Spikeone]

@ottml I updated the script files sligthly, maybe you can also update them here (https://github.com/Spikeone/RttR_Campaigns/tree/master/Campaign_OriginalRoman) - they now use your mappreview function (although this could have been a campaign setting, thinking about it)

[ottml]

@ottml I updated the script files sligthly, maybe you can also update them here (https://github.com/Spikeone/RttR_Campaigns/tree/master/Campaign_OriginalRoman) - they now use your mappreview function (although this could have been a campaign setting, thinking about it)

Updated to your version.

[ottml]

[Flamefire]

Awesome, I think this is ready now!

@ottml I rebased the branch squashing the fixup-commits into their originals (i.e git commit -i --autosquash master) and added a minor improvement on top to avoid you having to do another change after you already did so much here. I hope that is ok for you, otherwise feel free to do the squashing-rebase yourself and force-push your branch again.
Note: If you want/need to work on the updated (rebased) branch again you'll have the update your local branch: git fetch <your-remote> && git reset --hard <your-remote>/campaign_screen_final. But with your agreement I'll merge this as soon as CI has passed so you can also just delete that local branch and continue of the master after the merge.

Please tell me if this is OK for you to merge this with that. It is your work so I'm waiting for your OK or update here. Thank you!

[ottml]

Awesome, I think this is ready now!

@ottml I rebased the branch squashing the fixup-commits into their originals (i.e git commit -i --autosquash master) and added a minor improvement on top to avoid you having to do another change after you already did so much here. I hope that is ok for you, otherwise feel free to do the squashing-rebase yourself and force-push your branch again. Note: If you want/need to work on the updated (rebased) branch again you'll have the update your local branch: git fetch <your-remote> && git reset --hard <your-remote>/campaign_screen_final. But with your agreement I'll merge this as soon as CI has passed so you can also just delete that local branch and continue of the master after the merge.

Please tell me if this is OK for you to merge this with that. It is your work so I'm waiting for your OK or update here. Thank you!

@Flamefire It's OK for me to merge it. Thanks for the fast and productive review :)

[Flamefire]

@Flow86 Blocked by:

• https://github.com/Return-To-The-Roots/s25client/pull/1634