World-sized bitmaps were allocated with size WORLD_SZ() / 8, which
expands to (WORLD_X * WORLD_Y / 2) / 8. The divisions truncate unless
WORLD_X * WORLD_Y is a multiple of 16. The bitmaps were one byte too
small then. Bitmap overruns happen when:
* A lookout looks at one of the last sectors of the sector file.
Besides commands look and llook, this affects navigate and march
sub-command 'l'.
* Command spy spies into one of the last sectors of the sector file.
* A map or nmap (but not a bmap) shows one of the last sectors of the
sector file, or a sector that can see one of the last sectors
(visual range is two sectors at 100% efficiency). Besides commands
lmap, map, nmap, pmap, smap, this affects move and transport
sub-command 'm'.
Diagnosed with valgrind.
Already broken in BSD Empire 1.1 (bitmaps were on the stack then).
We know player != other. Because we can have only one player in state
PS_PLAYING per country, and we know other->state == PS_PLAYING, it
follows that player->cnum != other->cnum. Thus, no functional change.
Adds another call to getnatp() hidden in relations_with(), though.
Keeping that optimized isn't worth it.
Replacing getrel(NP, THEM), where NP is known to be getnatp(US), by
relations_with(US, THEM) makes a difference only when US equals THEM.
Replace in places where it's obvious that they're not equal.
Adds a few calls to getnatp() hidden in relations_with(). Keeping
that optimized isn't worth it.
Replace patterns like "US == THEM || getrel(NP, THEM)...", where NP is
known to be getnatp(US), by "relations_with(US, THEM)...". No
functional change.
Adds a few calls to getnatp() hidden in relations_with(), though.
Keeping that optimized isn't worth it.
No functional change, because the value of rel only matters when cn !=
victim, and then it's the same as before.
The new value of rel permits simplifying cn != victim && rel <=
NEUTRAL to just rel <= NEUTRAL
This removes a special case for POGO (#0). Before, unoccupied sectors
were treated as "own or allied" for POGO, but not for other deities.
Impact on callers:
* BestAirPath() is not affected, because the change is only reachable
with a non-null bigmap argument.
* sorde() and nav_ship() pass a non-zero ship owner. sorde() ensures
that itself, and prod_ship() does it for nav_ship().
* unit_path() passes the player number when called with a ship
argument, i.e. in the navigate command. Player number is zero for
POGO. Since deities can't navigate foreign ships, this can happen
only when POGO navigates dead ships. Yes, that's possible, needs
fixing.
* getpath() passes the player number (zero for POGO) when called with
argument P_SAILING, i.e. by the sail command.
Thus, the change makes navigate's and sail's path finding work for
POGO exactly like it does for other deities. That's fine.
No functional change, even though this changes rel[intruder] from
NEUTRAL to ALLIED. Uses of rel[]:
* getilists() and ac_encounter() compare rel[cn] to HOSTILE. No
change, because NEUTRAL and ALLIED are both greater than HOSTILE.
* ac_encounter() compares rel[cn] to ALLIED, but only when cn !=
plane_owner. Because it passes plane_owner as argument for
getilists() parameter intruder, rel[cn] can't refer to the changed
element of rel[] here.
The new value of rel[plane_owner] permits simplifying cn ==
plane_owner || rel[cn] == ALLIED to just rel[cn] == ALLIED.
No functional change, because the value of rel only matters when
sectp->sct_own != sp->shp_own, and then it's the same as before.
The new value of rel permits simplifying sectp->sct_own == sp->shp_own
|| rel >= FRIENDLY to just rel >= FRIENDLY.
No functional change, because the change affects only
notified[victim], which isn't used in the loop around
notify_coastguard(), and gets overwritten before the interdiction fire
loop.
No functional change, because the value of rel only matters when
sect.sct_own != actor, and then it's the same as before.
The new value of rel permits simplifying sect.sct_own != actor && rel
!= ALLIED to just rel != ALLIED.
Replacing getrel(getnatp(US), THEM) by relations_with(US, THEM) makes
a difference only when US equals THEM. Replace in places where it's
obvious that they're not equal.
Note: getsect() sets player->owner to "player is god or owns this
sector". Thus, after getsect(..., §), sect.sct_own ==
player->cnum implies player->owner. Conversely, !player->owner
implies sect.sct_own != player->cnum. Similarly for getship(),
getplane() and nxtitem().
Switching from getrel() to relations_with() can change the value from
NEUTRAL to ALLIED. The change doesn't matter when the value's only
compared to HOSTILE, as both old and new value are greater than
HOSTILE. Likewise for >= NEUTRAL.
Replacing getrel(getnatp(US), THEM) by relations_with(US, THEM) makes
a difference only when US equals THEM.
Replace patterns like "us == them || getrel(getnatp(us), them)..." by
"relations_with(us, them)...".
Relations checking with getrel() often needs a special case for "is
same country". If you forget, you get behavior appropriate for a
neutral foreign country, which is usually very wrong (see commit
16c68eb4 for an example).
Unlike getrel(), relations_with() considers countries allied to
themselves. Less dangerous. In fact, allied behavior is typically
just right, so the special case isn't even needed.
Before, it overwrote '?', '.', ' ' in the bmap with the capitalized
country letter, but only for sectors the player owns. Pretty
harmless, just weird. It can't happen currently, because sharebmap
with self fails with "does not have friendly relations towards you".
Flashing yourself failed with a bogus "not logged on" message for
deities, and a mildly bogus "not a deity or friendly with us" message
for mortals.
Fix by simply permitting it. Not terribly useful, except perhaps for
empire-hub users, but why not.
The second patch hunk fixes a latent bug. Before, rejected deity
flashes led to a bogus "not logged on" message, now they lead to a
"not accepting" message. But deity flashes can't be rejected, so this
doesn't matter.
sendmessage() checked NF_FLASH on two places: when deciding whether to
send the message, and later when telling the player why it didn't send
a flash. This can race with the toggle command as follows: if a flash
could not be sent because the recipient's NF_FLASH was off, and the
recipient toggled it on before the flag was checked again, the flash
command claimed the sender wasn't logged on.
feels_like_helping() case cn == foe is missing in the code it
replaces. No difference in behavior, because:
* cn == foe && cn == friend can't happen. Because you can't get into
ground combat against yourself (assault, attack and paradrop don't
let you), friend != foe for support.
* cn == foe && cn != friend behaves the same: no support.
feels_like_helping() returns 0 because of the explicit case. The
replaced code doesn't support because cn can't be at war with
itself.
Mission execution first builds lists of eligible units, one list per
country. These lists are passed to perform_mission() one by one,
where they get freed.
Bugs:
* unit_interdict() didn't pass the list for the submarine's owner, but
build_mission_list_type() built one. Any submarine movement within
own submarine interdiction mission op areas leaked memory.
* dosupport() passed only lists for countries that actually support
(ally at war with victim), but build_mission_list_type() built lists
for all countries hostile to the victim. Ground combat within
support mission op areas countries that are hostile to one of the
party without actually supporting the other leaked memory.
* perform_mission() failed to free missiles targeting units.
Fixing the latter is straightforward.
Fix the first two by deciding whether a country acts on a mission
trigger before building any lists, in ground_interdict(),
unit_interdict(), dosupport(). Remove the code dealing with that from
build_mission_list_type() and the loops around perform_mission().
lnd_mar() and lnd_mar_one_sector() take an actor argument.
Nevertheless, they sometimes used player->cnum. Fortunately, they are
the same: all callers pass current player for actor. Normalize to
actor for consistency.
Land units pay a mobility penalty when marching into a non-old-owned
sector without sector mobility, to slow them down in newly taken
sectors. Attacking land units pay this penalty regardless of sector
mobility.
When attacking out of an allied sector, the penalty was computed as if
the land unit was owned by that ally. Attacking sectors old-owned by
that ally was too cheap, and taking back one's own was too expensive.
Broken since attacking land units pay the "newly taken" mobility
penalty: commit 2e693275, v4.3.6.
When an attacking sector got lost while the player was at a prompt,
and the new owner was allied to the player, the server got confused:
1. If the sector attacked with mil, the server let the ghost mil
attack, but not occupy.
2. If the sector was allied, the server reported the sector loss and
land units dropping out of the attack, but claimed the lost sector was
yours.
Fix 1. by dropping sectors from attack when they change owner away
from the player, regardless of relations. Side effect: also drops any
surviving land units there. Before, they dropped out only if the new
owner wasn't allied to the player. That change's okay.
Fix 2. the obvious way: change the messages.
Broken in 4.0.0.
SLOW_WAR has issues:
* The check whether the attacker old-owns the attacked sector is
broken, because att_abort() uses sect.sct_oldown uninitialized.
Spotted by the Clang Static Analyzer.
* Its implementation in setrel() is somewhat scary. It's actually
okay, because that part of setrel() only runs within decl(). Other
callers don't reach it: update_main() because player->god != 0
there, and the rest because they never pass a rel < HOSTILE.
* Documentation is a bit vague.
SLOW_WAR hasn't been used in a public game in years. Fixing it is not
worth it, so remove it instead.
"rm -r" prompts for read-only files when stdin is a tty. Probably
broken since we install builtin configuration read-only, in commit
b4161cd7, v4.3.0.
Move "done" message to the right place. Broken in commit 70c03561,
v4.3.12.
