Follow-up Comment #18, patch #5306 (project freeciv):
I had a look and found the function player_capital() in player.h. It returns
the player's capital. Please decide if you prefer to send the expelled unit to
his capital (get the city from player.h's player_capital(unit_owner(target))
or to the city a diplomat would flee to (get the city from find_closest_city
like diplomat escape does).
As you probably have noticed I'm not as quick to respond as I was earlier this
week. I think I will be able to take time for a new review tomorrow afternoon.
Don't worry if you don't have a new iteration by then. I'll probably have time
for a review again early next week or maybe even at the end of the week end.
_Just a moment of sadness and anger._
I understand. Freeciv is a large code base. Even with tools that helps you
navigate it there is a lot of new stuff.
_The "order" or "disposition" is correct?_
Of do_expel_unit()? Mostly OK. See comments below.
+ Expel all the unit at pdesttile using actor.
Comment will need an update.
+ char expeller_link[MAX_LEN_LINK];
Who is this a link to? (Please rename for clarity)
+ struct tile *expeller_tile;
Who is supposed to be on this tile? (Please rename for clarity)
+ /* find closest city capital for expel target */
The code tries to find the city a diplomat will flee to. Pick the comment or
the code.
+ pcity = find_closest_city(expeller_tile, NULL, unit_owner(actor), FALSE,
+ FALSE, FALSE, TRUE, FALSE, NULL);
You got the owner of the wrong unit. You want to find a city belonging to the
target so you can send it there. (Or just go for the capital in stead)
+ if (pcity != NULL) {
Let us keep things simple. Abort if no city is found. (pcity == NULL means
that no city was found) Notify the actor player that there is no place to send
the target and insert a return statement. (You have written notice code
below)
+ if (pcity && is_capital(pcity)) {
Remove the capital test. If you decide to expel the target to his capital you
could add a fc_assert with is_capital() in stead. Since you aborted if pcity
wasn't found you don't have to test it.
+ if (!target) {
Not sure if I understand what you were trying to do here. Let me know if I got
you wrong.
+ /* Notify. */
+ notify_player(uplayer, expeller_tile, E_UNIT_LOST_EXPEL, ftc_server,
+ /* TRANS: <unit> ... <Poles> */
+ _("You don't have expelled %s."),
+ unit_link(actor));
This is the notice I mentioned above. English issue (I'm not a native
speaker): I think it should be "You didn't expel"
+ /* This may cause a diplomatic incident */
+ action_consequence_caught(ACTION_EXPEL_UNIT, pplayer, uplayer,
+ expeller_tile, expeller_link);
I don't think that not finding a place to send someone should count as getting
caught/stopped. I'm open to counter arguments.
Outside of do_expel_unit():
+ /* If actor_unit can do the action the actor_player can see how much
+ * gold target_player have. Not requireing it is therefore pointless.
Copy past error.
+ GEN_EV(E_UNIT_LOST_EXPEL, E_S_UNIT, N_("Expel Failed")),
+ GEN_EV(E_UNIT_DID_EXPEL, E_S_UNIT, N_("Expel Succeeded")),
The expulsion didn't fail when your own unit was expelled. The unit didn't die
so it wasn't lost. While the expulsion was a success the chance of failure
wasn't all that high. The question is "you did this" vs "someone did this to
you", not loss or success. Was your goal to avoid introducing a third event
type? Maybe you could introduce a generic unit failed event if you can't find
an exiting event that covers it?
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?5306>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Freeciv-dev mailing list
[email protected]
https://mail.gna.org/listinfo/freeciv-dev
