Update removeFromSquad to match in-game logic #5527
Conversation
aside from the link removal stuff at the end, which might still be slightly different
|
Note: I don't have any fortresses with which to test this change - please verify before merging. |
chdoc
left a comment
chdoc
left a comment
There was a problem hiding this comment.
Apart from the few nitpicks below, I have let autotraining run for a few months based on this implementation, and have not noticed any bad behaviors or crashes.
| unit->military.squad_id = -1; | ||
| unit->military.squad_position = -1; |
There was a problem hiding this comment.
How about moving this before the if (squad) and turning the test into an early return if the squad is not defined, reducing the nesting level by one.
| } | ||
| // remove entity squad link | ||
| df::historical_figure* hf = df::historical_figure::find(unit->hist_figure_id); | ||
| if (hf == nullptr || squad == nullptr) |
There was a problem hiding this comment.
| if (hf == nullptr || squad == nullptr) | |
| if (hf == nullptr) |
assuming the test for the squad is turned into an early return
There was a problem hiding this comment.
Because the in-game logic will still clear user->military.squad_id + user->military.squad_position even if the squad doesn't actually exist anymore.
I suppose the case could be made for clearing those variables earlier, but I'm also replicating the order in which the game's own logic does it.
There was a problem hiding this comment.
Yes, I wasn't suggesting to change the behavior. As it comes to the order of things, I don't see any volatile variables. So the real order is whatever the compiler makes of it, both for our code and the game code. I would prefer to see less nesting and not fetching the historical_figure if there is no squad. But then, this is all well and truly in the eye of the beholder. What I truly care about is that autotraining works, which it seems to do with his implementation.
3 participants
aside from the link removal stuff at the end, which might still be slightly different
Amends #5526