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 lobby desktop #1773

[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.

[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

[tehKaiN]

Oops, looks like this thing got buried under the rubble of my other activities, sorry! 👀

Is that thing still desired for the project? If yes, then I could get back to this #1536 soon.

[Flamefire]

Is that thing still desired for the project? If yes, then I could get back to this #1536 soon.

Sure. IIRC the main issue in that other PR was a clear definition of the behavior (also in corner cases). Best to be done in code (tests). But has been a while and would need to read up on it again when you work on it

[tehKaiN]

I've tweaked the field sizes a bit so that it looks better on 800x600:

The only problem is that AI (medium) doesn't fit the display. I could probably make some room by moving the map preview to the right, but that would also mean moving bottom controls so that they end at the same X pos.

The problem would be non-existant if we'd somehow get rid of "Ready?" column on top - perhaps adding a tooltip to the checkbox class? Nevermind that each row displays the checkbox - I've hacked it in for sake of preview.

Apart from that, setting the preferred portrait in options desktop works as intended, as well as (de)serialize in game config. I still need to check if those portraits survive the game save/load correctly.

EDIT: the column names are all over the place with some languages and sadly there's not much to do about them. But still, I guess that proper display of English/German is enough on such low resolution.

[Flamefire]

Savegames and replays are all incompatible due to a new field in the player info.

This indeed requires some kind of migration code.

Would you be up to adding such? I guess you'd need to start at GamePlayerInfo and go up from there.

Well, I guess the firest packet in communication could contain some kind of version field so that incompatible clients are rejected early on or they refuse to connect to too new servers? That would mean another breaking change if there's no field like that, but that would be future-proof.

I found we already require clients to have the same revision:

s25client/libs/s25main/network/GameServer.cpp

Lines 926 to 927 in ec289ff

	else if(msg.revision != rttr::version::GetRevision())
	typeok = GameMessage_Server_TypeOK::StatusCode::WrongVersion;

So that should be enough.

There is a new network message but it uses the ID of an existing message

Sounds like sloppiness /auto-merge break and shouldn't be the case!

Given it is not possible that a client communicates with an incompatible client I think we can keep the messages ordered by name as you had before.

I don't fully understand the change to dskOptions. What does this do?

Similar to gif in the issue page (with correct label right now, of course):

I see. Where is this used? I though portraits are only for ingame use and hence the selection only makes sense in the lobby screen.

[tehKaiN]

Would you be up to adding such? I guess you'd need to start at GamePlayerInfo and go up from there.

Of course!

Given it is not possible that a client communicates with an incompatible client I think we can keep the messages ordered by name as you had before.

Alright, I'll revert my change on my nearest commit spree.

I see. Where is this used? I though portraits are only for ingame use and hence the selection only makes sense in the lobby screen.

The portrait selection in options screen sets up the default portrait, so that if you create a game and want to play as Knut, you don't have to cycle through portraits each time.

[Flamefire]

Looks good! Final polishing then it's good to go 👍

[Flamefire]

One final place in need for range check I missed before

[Flamefire]

Took a lot of work, so thanks!