As non-visual replays have their own mod check it needs to be updated after rP21239. It still only checks for the mod name and not for the versions. (Style may be not the best, it's late and I should already be sleeping)
Edit: This diff also adds the ability to convert a vector of vectors of strings via ScriptConversions
Details
Details
- Reviewers
elexis - Commits
- rP21603: Fix mod check of non-visual replays and allow conversion of a vector of vectors…
- Trac Tickets
- #5044
Replay a newer replay non-visually by doing:
In the mods folder create a folder named testthis. In there create a mod.json with this content:
{
"name": "testthis",
"version": "2.23",
"label": "testthis",
"url": "example.com",
"description": "Test.",
"dependencies": [],
"type": "game"
}Start a match.
Then enable this mod start another match.
with each of them do:
binaries/system/pyrogenesis -replay=path_to_commands.txt -mod=public
binaries/system/pyrogenesis -replay=path_to_commands.txt -mod=public -mod=testthis
Diff Detail
Diff Detail
- Repository
- rP 0 A.D. Public Repository
- Lint
Automatic diff as part of commit; lint not applicable. - Unit
Automatic diff as part of commit; unit tests not applicable.
Event Timeline
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/66/display/redirect
Comment Actions
Replaying public mod replays with the public mod now works.
But replaying a public mod replay with delenda est launched or a delenda est replay without delenda est launched doesn't result in an error message while we read code for that.
Did I test incorrectly?
| source/ps/Replay.cpp | ||
|---|---|---|
| 180 ↗ | (On Diff #5873) | If we implement GUI-mod-detection, then we can omit this hardcoding, #5053. So ok to hardcode now. |
| source/scriptinterface/ScriptConversions.cpp | ||
| 312 ↗ | (On Diff #5873) | Conversion of a vector of a vector was needed, ok. |
Comment Actions
If this gets committed with the vector of vector js conversion thing, we should do some cleanup afterwards, as some other parts of the code can also use that.
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 312 ↗ | (On Diff #5873) | I did, but failed. But if you have an idea how, feel free to post it/try it. |
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 312 ↗ | (On Diff #5873) | Since we need to express the type explicitly that is converted and since we can't compile code of infinite size while there are infinitely many types (vectors of arbitrary stacking), it should be impossible to achieve this for all dimensions. But it should be possible to write a macro for vectors between 0 and 10 dimensions. |
Comment Actions
Tested again, everything works as expected from the feature.
Thanks for the fix!
If you don't add the mod version test, we should have a simple ticket for that.
| source/ps/Replay.cpp | ||
|---|---|---|
| 42 ↗ | (On Diff #5873) | #include <algorithm> for that std::find? |
| 171 ↗ | (On Diff #5873) | user mod is not saved anymore to replays as of rP21239, so removing it here is correct. |
| 176 ↗ | (On Diff #5873) | (std::includes does something different, couldn't find an easier condition) |
| source/scriptinterface/ScriptConversions.cpp | ||
| 312 ↗ | (On Diff #5873) | bump |
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 312 ↗ | (On Diff #5873) | Not really the time and the expertise to look into that. |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/260/display/redirect
Comment Actions
This works. Thanks for implementing the missing check on this end as well (that was missing since the beginning of nonvisual replays I suppose).
Tested with public mod and terra magna replay.
But there is one problem, it doesn't test for the order of mods launched, so either add the condition or a TODO or a simple ticket.
| source/scriptinterface/ScriptConversions.cpp | ||
|---|---|---|
| 312 ↗ | (On Diff #5873) | Could add wstring vector for consistency. |
Comment Actions
Can confirm it works on windows, lots of warnings without the patch, only relevant ones with it.
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/288/display/redirect
Comment Actions
Code looks good.
(We should also consider "GUI mods" which don't change the simstate and can be launched, triggering some false positives of these conditions here (wrong nr of mods, wrong order).
So wondering if it would make sense to just simplify that if it can't be entirely accurate now.
Then again meh, we don't have this kind of "GUI mod" support yet. (Should be either a JSON boolean or deriving it from the launched mods (which would cost performance or code complexity)).)
| source/ps/Replay.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #6238) | Is it a list? One thing I didn't like about the mod code was inconsistent naming. Sometimes these are labeled as a set (which it isn't as we see) or a sequence. |
| 124 ↗ | (On Diff #6238) | ret->text? |
| 125 ↗ | (On Diff #6238) | & |
| 128 ↗ | (On Diff #6238) | (Was thinking about JSON stringify, but less readable result than this) |
| 128 ↗ | (On Diff #6238) | When I was asking to move something to a new function, it wasn't these 3 lines but the entire hunk that does the mod comparison. |
| 192 ↗ | (On Diff #6238) | In the string it's enabled mods, in the variable names it's launched mods, gah. The enabled/launched/whatever mods don't match the mods of the replay? |
| 204 ↗ | (On Diff #6238) | s/This/These |
| 176 ↗ | (On Diff #5873) | does it like if (warn)? Ah, the leper standard was If (!warn.empty()) iirc |
| source/scriptinterface/ScriptConversions.cpp | ||
| 310 ↗ | (On Diff #6238) | thx (my consistency checker was wondering where brother CStrW and it's family std::vector<CStrW> went, but meh). |
| source/ps/Replay.cpp | ||
|---|---|---|
| 122 ↗ | (On Diff #6238) | A list that contains a list. Where does it says set? |
| 125 ↗ | (On Diff #6238) | true |
| 128 ↗ | (On Diff #6238) |
Just as you like. Functions are cheap. |
| 192 ↗ | (On Diff #6238) |
Ahm, no? In the var it is loaded mods ;P |
| 176 ↗ | (On Diff #5873) | if warn does not work. This is C++ ^^ |
Comment Actions
Successful build - Chance fights ever on the side of the prudent.
Link to build: https://jenkins.wildfiregames.com/job/differential/303/display/redirect
Comment Actions
Code looks good and it works as wished for. Thanks!
Shouldn't come from this patch, but it doesn't show a version for mods that don't exist, but the name:
These mods are enabled: public (0.0.23) this mod doesn't actually exist ()
| source/ps/Replay.cpp | ||
|---|---|---|
| 154 ↗ | (On Diff #6257) | elseif |
| 211 ↗ | (On Diff #6257) | good |
Comment Actions
pyrogenesis -replay.... -mod=public -mod=thismoddoesntexistbecausetheresatypointhename
Comment Actions
Ah, ok. So the existence of mods loaded via cli should be checked at some point in the code.