Portrait support

[tehKaiN]

fixes #798
fixes #1181

I still have unit tests and other stuff to fix, but I think it's ready for reviews.

There's small problem with column names on 800x600 resolution, but who plays using that res anymore?

This will be split into lesser PRs:

• Add lerp and inverseLerp helper functions #1535
• Add support for image cropping inside ctrlImageButton #1536
• clean up control ids in settings desktop

[tehKaiN]

the thing about lua test is funny:

/home/runner/work/s25client/s25client/tests/s25Main/integration/testWorld.cpp(261): fatal error: in "MapTestSuite/LoadLua": Unexpected log: Lua script did not provide the function getRequiredLuaVersion()! It is probably outdated.

on my local machine it prints space between function name and () - very strange

[ctest] D:/git/s25client/tests/s25Main/integration/testWorld.cpp(261): fatal error: in "MapTestSuite/LoadLua": Unexpected log: Skrypt Lua nie udostępnił funkcji getRequiredLuaVersion ()! Prawdopodobnie jest nieaktualny.

but only when I run it via ctest, when I run it manually it works fine. Perhaps some kind of log formatting from cmake?

[Flamefire]

Great work!

on my local machine it prints space between function name and () - very strange

This is a translation bug. Fixed by Return-To-The-Roots/languages@d465390

Besides that the PR is really big although the changes are related. This may make the review and fixing iterations a bit longer, but should work out. So don't mind the many comments.

[Flamefire]

Can you remove this logging? I don't think the ctor should log anything... Especially as you do that at https://github.com/Return-To-The-Roots/s25client/pull/1534/files#diff-05c8c279f8bb320e4f03b8887355e77653338bec600b7418fdcdc288f680e1b3R1073

I know other Messages do that too but it is still wrong IMO

[tehKaiN]

do you want me to remove it in all messages then?

[Flamefire]

If you want yes. Best do that in a separate PR though to keep the diffs small.
Generally: If you can factor out changes into self-contained parts better do more small PRs. Same for commits: Fixups (e.g. formatting) can go into the commit that introduced the issue as long as the PR is not yet merged (or not even reviewed). This makes it possible to revert a commit later on. So small, self-contained commits are good.

[tehKaiN]

ok, we'll do this separately then. That single removal is done.

[tehKaiN]

I'm a bit worried about breaking changes in serialization - I'm planning to tackle some more issues blocking single player campaign implementation and perhaps there will be more serialization-intrusive things along the way. Perhaps we should merge it into separate branch (something like campaign) and then merge in all the changes at once into master, breaking stuff only once?

[Flamefire]

I'm a bit worried about breaking changes in serialization

Then maybe factor out the other changes into separate PRs. E.g. the use of enum-IDs for the controls, the image-handling etc. So all that is left is really only the portrait.

If you don't know yet: git add -p allows to chose which changes to stage at the hunk level. So you can then commit only parts of the changes in a file. I found that very useful when splitting up commits.

[tehKaiN]

The thing is, image handling was required to make portraits in lobby in reasonable shape. Splitting settings and lobby portrait into separate PRs seems like an overkill to me. I can make lua thingy and IDs separate, but dunno if it's really worth it.

What I meant regarding breaking serialization is that with portrait change I introduce serialization version 10. Perhaps with future PRs I'll introduce another breaking changes, bumping it e.g. to 11, 12 and 13. Perhaps it's wise to cram up as many serialization-breaking changes to a single version bump (e.g. by preparing them on separate feature branch), so that nightly players won't have their save files unusable too often.

[Flamefire]

The thing is, image handling was required to make portraits in lobby in reasonable shape. Splitting settings and lobby portrait into separate PRs seems like an overkill to me. I can make lua thingy and IDs separate, but dunno if it's really worth it.

ca7a173 and 1367fcc potentially with tests can go into a separate PR, especially as there is a remark for the latter. IMO that is a standalone change worth of its own PR and maybe we can come up with a few (automatic or manual) tests to check that it works as expected.

The ID change could be extracted. Yes it might not be really worth it but the idea is to bring as much into master as possible. I'm even thinking about making a change to the Serializer so the portrait change can be backwards compatible.

[tehKaiN]

Ok, I'll split my changes into separate branches and we'll merge them in more granular manner - probably not today though.

[tehKaiN]

Somehow I've missed some of your comments - all are fixed now. I'll continue to split the PR into smaller ones and I guess we'll leave portrait part unmerged until we decide if save compatibility should be broken or add support so that it'll be prevented