Ticket #44738

Split create_unit_full() in two functions

Date d'ouverture: 2022-06-01 05:55 Dernière mise à jour: 2022-06-19 02:16

Rapporteur:
Propriétaire:
État:
Atteints
Composant:
Jalon:
Priorité:
5 - moyen
Sévérité:
5 - moyen
Résolution:
Fixed
Fichier:
3

Détails

For consistency, we should have an option to do separately two things that create_unit_full() does together and mixes up:

  • Create a virtual unit, supply all necessary parameters to it and
  • Put a preset virtual unit into the game, recording it in all the lists and structures.

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.

Ticket History (3/14 Histories)

2022-06-01 05:55 Updated by: ihnatus
  • New Ticket "Split create_unit_full() in two functons" created
2022-06-01 05:56 Updated by: ihnatus
  • Details Updated
2022-06-07 03:54 Updated by: ihnatus
Commentaire

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.

2022-06-08 10:18 Updated by: cazfi
Commentaire

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.


+ sz_strlcpy(victim_link, utype_name_translation(unit_type_get(pvictim)));

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.

- 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")

2022-06-08 15:17 Updated by: ihnatus
Commentaire

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".

2022-06-15 05:04 Updated by: ihnatus
Commentaire

Made a new patch, limited the application to unit_change_owner() (modified in my previous patch) and place_partisans().

2022-06-15 09:43 Updated by: cazfi
  • Propriétaire Update from (Aucun) to cazfi
  • Résolution Update from Aucun to Accepted
  • Jalon Update from 3.0.3 (atteints) to 3.1.0
2022-06-17 15:23 Updated by: cazfi
  • Summary Updated
2022-06-17 16:56 Updated by: cazfi
Commentaire

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.

2022-06-19 00:05 Updated by: cazfi
Commentaire

Root cause of #44847 has been figured out, and it has nothing to do with this patch (or any other in testing).

2022-06-19 02:16 Updated by: cazfi
  • État Update from Ouvert to Atteints
  • Résolution Update from Accepted to Fixed

Attachment File List

Modifier

You are not logged in. I you are not logged in, your comment will be treated as an anonymous post. » Connexion