... when referring to a function's parameter or a struct/union's
member.
The idea of using FOO comes from the GNU coding standards:
The comment on a function is much clearer if you use the argument
names to speak about the argument values. The variable name
itself should be lower case, but write it in upper case when you
are speaking about the value rather than the variable itself.
Thus, "the inode number NODE_NUM" rather than "an inode".
Upcasing names is problematic for a case-sensitive language like C,
because it can create ambiguity. Moreover, it's too much shouting for
my taste.
GTK-Doc's convention to prefix the identifier with @ makes references
to variables stand out nicely. The rest of the GTK-Doc conventions
make no sense for us, however.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Every piece is formatted either with pr(), or with sprintf() for later
sending with wu(). The output is actually identical. Format with
sprintf() always, and then either pr() or wu() the results.
While there, change the first parameter's type from int to natid.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Much of the retreat code duplicates navigate and march code. Worse,
retreat's version is full of bugs:
* Land units can sometimes retreat when they couldn't march: while on
the trading block (forbidden with march since 4.0.9), crewless
(likewise since 4.0.0), kidnapped in a foreign sector (inconsistent
since land units were added in Chainsaw 3), loaded on a ship
(likewise) or a land unit (inconsistent since trains were added in
4.0.0).
* Ships can retreat while on the trading block (forbidden with
navigate since 4.0.9)
* Land units can't retreat into foreign sectors even though they could
march there, namely when sector is allied or the land unit is a spy.
They can march there since 4.0.0.
* Land units keep their fortification on retreat. Has been that way
since retreat was added in Chainsaw.
Then there's group retreat. It's basically crazy:
* It triggers retreat for everyone in the same fleet or army, one
after the other, regardless of retreat path, conditions (including
group retreat), or even location. The latter is quite abusable
since retreats aren't interdicted. Has been that way since retreat
was added in Chainsaw.
* Group retreat fails to trigger when the originally retreating ship
or land unit has no retreat path left when it's done. Broken in
commit b860123.
Finally, the reporting to the owner is sub-par:
* When a retreat is cut short by insufficient mobility or
obstructions, its end sector isn't reported, leaving the player
guessing.
* Non-retreats can be confusingly reported as retreat to the same
sector. Can happen when the retreat path starts with 'h' (obscure
feature to suppress a single retreat), or when a group retreat
includes a ship or land unit without retreat orders.
* Interaction with mines during retreat is reported before the retreat
itself, which can be quite confusing.
* Sweeping landmines isn't reported at all.
* Much code and much bulletin text is dedicated to reporting what
caused the retreat, even though it should be perfectly obvious.
Rewrite this on top of common navigate and march code. Reuse of
common code fixes the "can retreat when it couldn't navigate/march"
and the "can't retreat into sectors it could navigate or march into"
bugs, and improves the reporting.
One special case isn't a bug fix but a rule change: mountains. The
old code forbids that explicitly, and it's clearly intentional, if
undocumented. The new code allows it by not doing anything special.
Turn group retreat into an actual group retreat: everyone in the same
fleet and sector with the the same retreat path and group retreat
condition joins the group. The group retreats together, just like in
navigate and march.
Take care to always report the end sector. When retreat is
impossible, report "can't retreat". When retreat is partial, report
"and stays in X,Y". When it's complete, report "stopped at X,Y".
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Most of the remaining callers need to fool around with snprintf() to
use them. Not worthwhile anymore.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
We stop on mine hits only when they're fatal. Has always been that
way. When interdiction was added in Chainsaw, it worked the same.
Empire 2 changed the commands to stop on any interdiction damage. Now
stop on any mine damage, too.
Interdiction can fail to do damage (all bombs miss), and mines can be
detected without damage (by sweeping). Perhaps we should stop then as
well. Left for another day.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
unit_move() is too big and has too many paths through its loop.
Maintenance of the (unspoken) loop invariant isn't obvious. In fact,
it isn't maintained on some paths. I found several bugs:
* We check prerequisite conditions for moving before the first move
and around prompts. When a condition becomes wrong on the move,
movement continues all the same until the next prompt. I believe
the only way this can happen is loss of crew due to hitting a mine.
* We cache ships and land units in a list of struct ulist. When a
ship or land unit gets left behind, its node is removed from the
list and freed.
We keep pointer flg pointing to the flagship in that list for
convenience. However, the pointer isn't updated until the next
prompt. It's referenced for automatic radar and all sub-commands
other than the six directions and 'h'. Use after free when such a
sub-command gets processed after a flagship change without a prompt.
Same for land units. For instance, navigating a pair of ships "jh"
where the flagship has no mobility leaves the flagship behind, then
attempts to radar automatically using the ship in the freed list
node. Likewise, marching a similar pair of land units "jl" examines
the land unit in the freed list node to figure out how to look.
* We cache mobility in the same list to support fractional mobility
during movement. Movement deducts from cached mobility and writes
the result back to the ship or land unit.
If something else charges it mobility while it's in this list, the
cache becomes stale. shp_nav() and lnd_nav() reload stale caches,
but don't run often enough. For instance, when a ship hits mines,
the mine damage makes the cache stale. If a direction or 'h'
follows directly, the stale mobility is written back, clobbering the
mine hit's mobility loss.
This mess dates back to Empire 2, where it replaced a different mess.
There may be more bugs.
unit_move()'s complex control flow makes reasoning about its loop
invariant too error-prone. Rewrite the mess instead, splitting off
sensible subroutines.
Also fixes a couple of minor annoyances:
* White-space can confuse the parser. For instance, "jg l" is
interpreted like "jgll". Fix to reject the space. Broken in commit
0c12d83, v4.3.7.
* The flagship uses radar automatically before any sub-command (since
Chainsaw), and all ships use it automatically after a move (since
4.2.2). Make them all use it before and after each sub-command,
whether it's a move or not.
* Land units don't use radar automatically. Make them use it just
like ships.
* Always report a flagship / leader change right when it happens, not
only before and after a prompt.
Left for another day, marked FIXME: BTU charging is unclean.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Unlike the other "may move" conditions, "same mobility type
(MOB_MARCH, MOB_RAIL) as leader" and "not on sale" are only checked
when collecting ships and land units.
The former breaks when a deity unwisely edits a land unit's type while
it is being marched. Messed up when the check was added in commit
36e41e5 (v4.3.7). However, editing has become possible only recently,
in commit 6b0b6f1.
The latter would break if a ship could be put on the market while it
is being navigated, but that's not possible, because only the owner
can navigate (see also commit 8c502d4), and only the owner can sell.
Same for land units. Messed up in 4.0.9. Clean it up anyway. Bonus:
nicer error message, even spelled correctly.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
navi() uses shp_sel() to collect ships, then shp_nav() to drop
ineligible ships. shp_sel() wipes mission and retreat orders. Stupid
when shp_nav() will drop them right away.
Avoid that by having shp_sel() check shp_nav()'s conditions, too.
navi()'s shp_nav() call won't find anything to drop now. The call
will be removed shortly.
This drops "& stays in" from some failure reports, since shp_nav()'s
reject messages end with "& stays in X,Y", and shp_sel()'s don't.
Likewise for marc(), lnd_sel(), lnd_mar().
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Don't wipe it in lnd_sel(), rely on lnd_mar_one_sector() and
lnd_sweep() to wipe it when the land unit actually moves.
Closes FRE#43.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
lnd_may_mar() uses lp->lnd_own rather than actor, but that's okay,
lnd_mar() ensures they're the same.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Compare with DIR_LAST instead of DIR_VIEW, to avoid assuming DIR_VIEW
is the first non-direction code.
While there, oops on unexpected code.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
shp_sweep() and lnd_sweep() print only a couple of "Sweep...".
Sometimes, the sector isn't obvious, e.g. when you march multiple
sectors in one go, sweeping along the way.
Print "Approaching minefield at X,Y..." right before the first sweep
in a sector.
Note that retreat.c duplicates the sweeping code. Retreating ships
report sweeping with coordinates since commit dcd0794, v4.2.21.
Retreating land units still sweep silently. Left for another day.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Don't report every incapable ship or land unit. Complain only when
there are no capable ships or land units available.
The ships are all in the same sector, so complain about the sector
type just once instead of once per capable ship or land unit.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
When ships enter a sector with sea mines, any minesweepers sweep, then
hit mines, and finally all ships (including the minesweepers) hit
mines. Sweeping in a sector (navigate sub-command 'm') works the same
without the final step.
When land units enter a sector with land mines, any engineers sweep,
and then all land units (including the engineers) hit mines. Sweeping
in a sector (march sub-command 'm') works the same, which means
non-engineers can hit mines then. Broken in Empire 2.
Actually broken for ships too then. 4.0.17 fixed ships, but neglected
to fix land units.
Change the land unit code to work like the ship code. Fixes march
sub-command 'm' not to expose non-engineers to mines. Changes march,
attack and assault with option INTERDICT_ATT enabled to expose
engineers twice.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
When lnd_sweep() rejects an engineer for want of mobility, it removes
it from the list of units.
Can happen only when sweeping for march sub-command 'm'. Any engineer
without mobility is dropped from the march immediately. Broken in
Empire 2.
Fix lnd_sweep() to handle this case just like the others, and like
shp_sweep(): report and continue with the next list member.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Require positive mobility for sweeping mines, just like ships do.
Screwed up when land units were added in Chainsaw 3.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Commit cd8d742 mechanically combined struct mlist's mcp and struct
llist's llp into struct ulist's chrp, adding type casts to every use.
Not necessary, simply use mchr[] and lchr[] directly.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Handle "no movement" before the movement loop instead of relying on
the first iteration of the loop.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The capability to march land units spread over several sectors is
obscure and rarely useful. Accidental use is probably more frequent
than intentional use. Issues:
* Interactive prompts show only the leader's position, and give no
clue that some land units are actually elsewhere.
* Path finding is supported only when all marching land units are in
the same sector.
* In each step, the bmap is updated for the leader's radar. The bmap
is not updated around other marching land units. Already odd when
all units are in the leader's sector, and odder still when some are
elsewhere.
* Interdiction becomes rather complex. For each movement, every
sector entered is interdicted independently. This means the same
ship, land unit or plane can interdict multiple times. Interdiction
order depends on the order the code examines land units. which the
player can control. This is all pretty much undocumented.
* Complicates the code and its maintenance. Multiplies the number of
test cases needed to cover march.
I feel we're better off without this feature.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Unlike the move command, march checks sector abandonment before every
step.
If the player declines, the last land unit stays put and is removed
from the march.
Except when sectors or land units change while we're waiting for the
player's reply. Then the last unit is not removed from the march.
This can scatter land units. Screwed up when checking for abandoning
the sector was added in 4.2.2.
Change march to work like move, and to avoid scattering land units: if
the player declines to abandon the sector, the command simply fails.
Put the check into new lnd_abandon_askyn().
Extend would_abandon() and want_to_abandon() from a single land unit
to many. Rename the latter to abandon_askyn() for consistency.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
When attempting to enter a sector with a land unit that can't go there
while the marching land units are all in the same sector, march stops
and prompts without removing the incapable land unit from the group.
If another land unit has already entered the sector, the group becomes
scattered.
This can happen when marching a mixed group of spies and non-spies
into a non-allied sector. Same for marching a mixed group of trains
and non-trains into a sector without rail, except such groups have
been disallowed since commit 36e41e5 (v4.3.7). Both screwed up when
spies and trains were added in 4.0.0
Remove the incapable land unit from the group when another land unit
can enter the sector. This avoids scattering land units.
Don't remove incapable land units when no land unit can enter the
sector. Without this, march would remove everyone and end then.
It can also happen when sectors or land units change while we're
sitting at the "Do you really want to abandon X,Y" prompt. I'm going
to fix that differently.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
With autonav and SAIL gone, shp_nav_put() isn't used externally
anymore. lnd_mar_put() never was; it got external linkage just for
symmetry.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
lnd_put() serves two masters: march, which wants it to report
"stopped" and write back struct ulist member mobility to
unit.land.lnd_mobil, and ground combat, which doesn't.
lnd_put() assumes march when actor is non-zero. Correct (but see
commit 8c502d4). Dates back to Empire 2.
Too ugly for my taste. Specialize for each master instead:
lnd_mar_put() for march, and lnd_put() for ground combat.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Commit d94d269 combined them into unit_put(), but that has turned out
not to be useful. Split them again.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
... get_dlist(), att_reacting_units().
This loses malloc() error checking in ask_olist() and get_dlist(). No
great loss, because we don't check in so many other places, including
att_reacting_units(). We should use a wrapper that terminates on
error, though. Left for another day.
ask_olist(), get_dlist() and att_reacting_units() zero the struct
ulist with memset(). lnd_insque() doesn't, so these functions need to
zero any members not otherwise initialized explicitly now.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Change chance in percent lnd_retreat - lnd_effic - 1 to lnd_retreat -
lnd_effic. It's been that way since Empire 2, but I can't bring
myself to document the silly -1.
"info morale" wasn't updated when the retreat chance was changed in
Empire 2. Fix that.
March code reads the land units into a land unit list, and writes them
back when it changes them, e.g. when a land unit stops. If a land
unit changes in the land unit file while it is in such a land unit
list, the copy in the land unit list becomes stale, and must not be
used.
To that end, do_unit_move() calls lnd_mar() after prompting for path
or destination. lnd_mar() re-reads all the land units.
Unfortunately, it still writes back stale copies in certain
circumstances. Known ways to trigger such writes:
* Deity loads land unit onto a ship or land unit
* Land unit's crew killed just right, e.g. by collateral damage from
interdiction, followed by additional updates, such as shell fire
damage
* Sector no longer owned or allied, e.g. allied sector captured by an
enemy (own sector would kill or retreat the land unit)
Writing a stale copy wipes out the updates that made the copy stale,
and triggers a seqno mismatch oops. For instance, damage that follows
killing of all crew by collateral damage from interdiction is wiped
out. If no damage follows, we still get a generation oops.
lnd_take_casualty() uses uninitialized rsect to compute the mobility
cost of retreating a defending land unit. This can charge incorrect
mobility, prevent retreat, or, if the stars align just right, crash
the server when sector_mcost() subscripts dchr[] with it.
Broken in commit 4e7c993a, v4.3.6. Reported by Scott C. Zielinski.
