Made the patches. Applied them to unit_change_owner(), removing some redundant updates, fixing the comments and making this function safe to return NULL (that it could do before).
Some rebase of #44312 patches are needed.
Some parts seem like unrelated bugfixes worth their own tickets, such as using unit_list_iterate_safe() in do_capture_units() and api_edit_create_unit_full() checks. Those fixes should go to all branches, but the main change is very complicated (-> high risk of introducing bugs in some corner cases), so I don't think it belongs to a mature release branch.
Really minor, but I would only store unit type ( unit_type_get(pvictim) ) here, and do the rest afterwards only if this fallback is really needed.
At least in the patch context this initialization value is not used. (We have several reasons *not* to set initialization value when it's not meant to be used. For one, it masks the fact that the value is not 'correct' from valgrind. Also clang analyzer is likely to complain about "dead assignment")
Reply To cazfi
Some parts seem like unrelated bugfixes worth their own tickets, such as using unit_list_iterate_safe() in do_capture_units() and api_edit_create_unit_full() checks. Those fixes should go to all branches, but the main change is very complicated (-> high risk of introducing bugs in some corner cases), so I don't think it belongs to a mature release branch.
Ok, I'll rework it only for 3.12. The script-safe call of d_c_u actually might get its own ticket as it goes a bit beyound #44312. Rewriting the mechanism completely for HRM874201 is unlikely before 3.2 due to compatibility issues.
- bool bounce; + bool bounce = FALSE; At least in the patch context this initialization value is not used. (We have several reasons *not* to set initialization value when it's not meant to be used. For one, it masks the fact that the value is not 'correct' from valgrind. Also clang analyzer is likely to complain about "dead assignment")
Compiler required me to do this.There is a use in the end of the function, out of the "if".
Made a new patch, limited the application to unit_change_owner() (modified in my previous patch) and place_partisans().
It might be a coincidence that #44847 occurred at the autogame run where this patch was first included, but I'll keep this on hold a bit longer, until we know more about the failure.
Root cause of #44847 has been figured out, and it has nothing to do with this patch (or any other in testing).
For consistency, we should have an option to do separately two things that create_unit_full() does together and mixes up:
The downside of what is now is that in e.g. unit_change_owner() we create a unit, put it into the game and then make some raw adjustments, e.g. set its nationality and movepoints. We call AI callbacks before we do it; if now they don't interest these parameters, afterwards they may.
Actually, I have in mind a pair of patches for later versions that need it, but even for 3.0 it would be better.