Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
476 stars 75 forks source link

Portrait support #1534

Open tehKaiN opened 1 year ago

tehKaiN commented 1 year ago

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?

obraz

This will be split into lesser PRs:

tehKaiN commented 1 year ago

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?

tehKaiN commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

https://github.com/Return-To-The-Roots/s25client/pull/1534/commits/ca7a173d8971dc781fcf19b1038c61556dfca7ad and https://github.com/Return-To-The-Roots/s25client/pull/1534/commits/1367fccc4e3960859627dfe80b5da1dddc8d67a9 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 commented 1 year ago

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

tehKaiN commented 1 year ago

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