Return-To-The-Roots
Wine addon
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 :)
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.
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 https://github.com/Return-To-The-Roots/s25client/blob/7bf8281a41e094ad1aa43d574da489a13afe1958/libs/s25main/GamePlayer.cpp#L280 is the issue as you increased the transportPrio array size. Above that is an example how to handle it.
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
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 :)
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
https://github.com/Return-To-The-Roots/s25client/blob/7bf8281a41e094ad1aa43d574da489a13afe1958/libs/s25main/GamePlayer.cpp#L280 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.
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. https://github.com/Return-To-The-Roots/s25client/blob/7bf8281a41e094ad1aa43d574da489a13afe1958/libs/s25main/Replay.cpp#L23
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
lastGFentry 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.
https://github.com/Return-To-The-Roots/s25client/blob/7bf8281a41e094ad1aa43d574da489a13afe1958/libs/s25main/Replay.cpp#L23
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.
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
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?
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 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
After the versioned serializer was merged you should be able to get the replay tests passing with this commit
Note: Just pick this single commit or the patch, not the branch or C&P the changes from that commit
@Flamefire The patch fixes the crash but during execution there is a mismatch in the checksum. Any idea what could be the problem here?
Because we have now 3 jobs more the random number generator is not in sync anymore. This happens for example in the loop in BurnedWarehouse.cpp:76. Calling the random generator at BurnedWarehouse.cpp:90 three times more. The replay can be executed but a warning is shown. The test itself fails.
A potential fix could be adding:
if(count == 0 && wineaddon::isWineAddonJobType(job))
continue;
Sorry for the delay, I've been on vacation. You are right with your evaluation and I found that too. I had already fixed that earlier and made a PR in your fork: https://github.com/ottml/s25client/pull/1
Your idea is good, but too specific to keep. My attempt basically skips every job type without people in the warehouse, just as yours but not restricted to the wine addon.
It also contains some improvements to the farmhand class which helps for this PR, e.g. avoiding a costly re-computation.
It does require a new test replay (not too bad given we broke that just recently) but should be a) faster and b) more stable going forward.
I rebased that verifying the replays work, so check that PR and review if you like. It is large, because I translated and fixed many comments and docstrings. It also contains an important bugfix where a figure could leave a warehouse in an invalid direction.
If you want you can also squash some of your commits (92 commits ...) here before merging, e.g. the 3 door constant commits or adjacent format/clang-tidy commits if you can't easily combine those with the earlier commit.
If all ci checks succeed i will do a last squashing of some commits to have a cleaner history.
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 :)
