Skip to content

Commit

Permalink
Fix my fix to my fix (fixup 0990613)
Browse files Browse the repository at this point in the history
@gfgtdf

Turns out I forgot to consider the old village being unassigned, which would
make the exception on getting the old team a common occurrence. I've changed
the code to continue on if it can't unassign the village.

I also noticed a slight semantic change: unassignment of the old side used to
be skipped if the new was defeated, but now it's not. Dunno if that's a big deal.
  • Loading branch information
Vultraz committed Feb 15, 2018
1 parent 0990613 commit 908b936
Showing 1 changed file with 8 additions and 15 deletions.
23 changes: 8 additions & 15 deletions src/scripting/game_lua_kernel.cpp
Expand Up @@ -1145,28 +1145,21 @@ int game_lua_kernel::intf_set_village_owner(lua_State *L)
return 0;
}

team* old_side = nullptr;
team* new_side = nullptr;

try {
old_side = &board().get_team(old_side_num);
board().get_team(old_side_num).lose_village(loc);
} catch(const std::out_of_range&) {
// old_side_num is invalid (which should really never happen), exit.
return 0;
// old_side_num is invalid, most likely because the village wasn't captured.
}

old_side->lose_village(loc);

try {
new_side = &board().get_team(new_side_num);
team& new_side = board().get_team(new_side_num);

if(!board().team_is_defeated(new_side)) {
new_side.get_village(loc, old_side_num, (luaW_toboolean(L, 4) ? &gamedata() : nullptr));
}
} catch(const std::out_of_range&) {
// new_side_num is invalid, exit. This also covers the case where it equals 0,
// new_side_num is invalid. This covers the case where it equals 0,
// which is supposed to only un-assign the side (done above).
return 0;
}

if(!board().team_is_defeated(*new_side)) {
new_side->get_village(loc, old_side_num, (luaW_toboolean(L, 4) ? &gamedata() : nullptr));
}

return 0;
Expand Down

6 comments on commit 908b936

@gfgtdf
Copy link
Contributor

@gfgtdf gfgtdf commented on 908b936 Feb 15, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also noticed a slight semantic change: unassignment of the old side used to
be skipped if the new was defeated, but now it's not. Dunno if that's a big deal.

well i don't really have an opinion on which behviour is better, but unless you have good reason to change it it should not be changed.

more importantly, it still has a bug: below it uses a 4-th arument but never a 3-th argument.

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't feel it's worth reverting to the old behavior on that one point.

@GregoryLundberg
Copy link
Contributor

@GregoryLundberg GregoryLundberg commented on 908b936 Feb 16, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Vultraz You asked:

How many fixes would a good fix fix?

It would fix and fix as much as it could, if a good fix could fix good.

@CelticMinstrel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @gfgtdf on restoring the old behaviour (where attempting to assign to a defeated side has no effect).

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

God dammit all to hell.

@Vultraz
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

HERE: 1196225

Please sign in to comment.