Wine addon

[ottml]

Fixes #1669

[Flamefire]

Just took a quick look for ideas and potential issues and improvements

[ottml]

I think the implementation is now feature complete. I will provide a new build in the next days for testing.
The only open task is:

just asking: If the addon is disabled, are all the icons (inventory, settings screen) hidden as well? If so, could you also implement it for the charburner as well, if not, could you implement that since using the default s2 settings should not show these icons so player can get an original feeling :)

[Flamefire]

The only open task is:

just asking: If the addon is disabled, are all the icons (inventory, settings screen) hidden as well? If so, could you also implement it for the charburner as well, if not, could you implement that since using the default s2 settings should not show these icons so player can get an original feeling :)

That would have been my question too: When the addon is disabled everything should look and work as before. I guess having the wares in the internal lists such as distribution is fine as it won't affect gameplay without those wares but for inventory screen, settings screen etc. it might matter.

[Flamefire]

Awesome work implementing this feature! Just some minor remarks but looks fine in general!

[Flamefire]

The failing replay test indicates you need to a) increase the gamedataversion and b) add compatibility code to allow loading older save games.

From a quick look it seems that

s25client/libs/s25main/GamePlayer.cpp

Line 280 in 7bf8281

helpers::popContainer(sgd, transportPrio);

is the issue as you increased the transportPrio array size. Above that is an example how to handle it.

[Flamefire]

Remove iwTempleBuilding completly and add stuff to iwBuilding

What was the reason for this? I think the combination of this with the previous is the best:

• Default size parameter for iwBuilding
• A subclass for the temple which passes the custom/larger size to iwBuilding
• subclass adds extra controls as required
• "Relative" coords for controls (as in the new commit)

This removes the need for the seemingly random extend parameter for the temple window.

I mean the file for the subclass is less than 50 lines with only the changes required for it which looks very clean

[ottml]

Remove iwTempleBuilding completly and add stuff to iwBuilding

What was the reason for this? I think the combination of this with the previous is the best:

* Default size parameter for `iwBuilding`

* A subclass for the temple which passes the custom/larger size to `iwBuilding`

* subclass adds extra controls as required

* "Relative" coords for controls (as in the new commit)

This removes the need for the seemingly random extend parameter for the temple window.

I mean the file for the subclass is less than 50 lines with only the changes required for it which looks very clean

This was a misunderstanding. I changed it accordingly :)

[Flamefire]

This was a misunderstanding. I changed it accordingly :)

Alright. Could have been that the implementation turned out harder than I thought or blocked by something. Hence the question :)

[ottml]

The failing replay test indicates you need to a) increase the gamedataversion and b) add compatibility code to allow loading older save games.

From a quick look it seems that

s25client/libs/s25main/GamePlayer.cpp

Line 280 in 7bf8281

helpers::popContainer(sgd, transportPrio);

is the issue as you increased the transportPrio array size. Above that is an example how to handle it.

I added compatibility code for old savegames. But the replay test does not use this code. Does the replay not only record GameCommands? Is there also a possibility for compatibility for GameCommands? For example the ChangeDistribution Command fails.

[Flamefire]

I added compatibility code for old savegames. But the replay test does not use this code. Does the replay not only record GameCommands? Is there also a possibility for compatibility for GameCommands? For example the ChangeDistribution Command fails.

I don't see a way to do this, the GameCommands use the basic serializer because they are used in the game where versioning doesn't make sense. While we could change that such that a version is added for those (and only those) we'd need to add the version to the replay file. And I cant see where/how without breaking backwards compatibility. Maybe we can use the lastGF entry and make it negative until raising the replay version to denote that the replay sub-version follows.
We better do that in a separate PR, I can look into that.

Otherwise we need to declare a new version rendering previous replays unusable.

s25client/libs/s25main/Replay.cpp

Line 23 in 7bf8281

return 8;

[ottml]

I added compatibility code for old savegames. But the replay test does not use this code. Does the replay not only record GameCommands? Is there also a possibility for compatibility for GameCommands? For example the ChangeDistribution Command fails.

I don't see a way to do this, the GameCommands use the basic serializer because they are used in the game where versioning doesn't make sense. While we could change that such that a version is added for those (and only those) we'd need to add the version to the replay file. And I cant see where/how without breaking backwards compatibility. Maybe we can use the lastGF entry and make it negative until raising the replay version to denote that the replay sub-version follows. We better do that in a separate PR, I can look into that.

Otherwise we need to declare a new version rendering previous replays unusable.

s25client/libs/s25main/Replay.cpp

Line 23 in 7bf8281

return 8;

If you think it's worse the effort we can do this. I think we should simply increase the version and live with the breaking loading of old replays, as lat done in 2023.

[Flamefire]

If you think it's worse the effort we can do this. I think we should simply increase the version and live with the breaking loading of old replays, as lat done in 2023.

Yes I think it is worth it and I already have a good idea how to do that. One of the reasons is that there are still some open issues with replays attached which are required to reproduce the issue. Breaking compatibility will make that impossible

I'll work on it over the weekend

[Flamefire]

Just some style improvements :)

[ottml]

If you think it's worse the effort we can do this. I think we should simply increase the version and live with the breaking loading of old replays, as lat done in 2023.

Yes I think it is worth it and I already have a good idea how to do that. One of the reasons is that there are still some open issues with replays attached which are required to reproduce the issue. Breaking compatibility will make that impossible

I'll work on it over the weekend

@Flamefire Are you working on this topic? Or do we increase the version for now and record the replays with the new version?

[ottml]

The implementation is finished. Only the topic in the above comment is unresolved. After this is finished i will provide a last test version to @aztimh. If everything is fine we can squash the commits into one and rebase the master.

[Flamefire]

@Flamefire Are you working on this topic? Or do we increase the version for now and record the replays with the new version?

Yes and almost done testing. See #1677
I'm currently testing whether we can have your addon and old replays working. Ran into an async related to the RNG

[ottml]

Squash finished. We can merge it as soon as ci is through.

[Flamefire]

🥳 🎉

[aztimh]

Awesome work thanks so much! I'll get some screenshots and write up a news post for Spike to put on the RTTR website. I'll be sure to credit all the work you both did programming and refining the code :)

Thanks again,

• aztimh :)