retreat: Rewrite automatic retreat code to fix its many bugs
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>
setup-POGO unintentionally gives dead ships, planes and land units
mobility, which makes them show up in final.xdump. Rearrange to avoid
that, and for clarity. While there, improve comments.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
edit: Fix tech and range adjustment for edit p key 'T'
It sets the new type, then falls through to setting tech if the new
type requires more than the plane currently has. Two problems with
that:
* If we fall through, the plane is invalid: it has less tech than
required. Its only use before it gets overwritten is pln_set_tech()
calling pln_range_max() to find out whether the range is limited.
Passes a negative number to log(). Not fatal, but pln_set_tech()'s
range adjustment is unlikely to work.
* If we don't fall through, the range may still need adjustment,
either up (to keep it unlimited if the new type has more range), or
down (to keep it within the new type's shorter range).
Screwed up when the key was added in commit 6b0b6f17. Fix by
adjusting tech first, then setting the type, then adjusting the range.
The latter relies on pln_set_tech() coping with ranges exceeding the
type's maximum, which it does.
Change the other type edits similarly for consistency.
When a type edit triggers a tech change, the tech change is now
silent.
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>
navigate march: Fix "same mob type" and "not on sale" checks
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>
navigate march: Wipe mission and retreat orders less eagerly
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>
bomb fire launch torpedo: Don't disclose ship sinking in retreat
These commands report "sunk!" even when the ship survives the attack
but sinks during retreat. bomb even reports where on the retreat the
ship sinks. Has been that way since retreat was added in Chainsaw.
Report "sunk!" only when the attack sinks the ship directly.
Similar code exists for land units, but it doesn't report killings.
Change it anyway, to keep it consistent with the ship code.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
.SY claims all arguments are individually optional. Fix that, and
clarify that omitting the optional arguments shows current orders.
Don't complicate syntax with <SECTS>, <SHIP/FLEET> covers sectors.
The two pages are identical apart from header and footer. They
mention land units briefly, then explain ship retreat. Lazy. Drop
the land unit mention from "info retreat", and rewrite "info lretreat"
for land units.
Update example output for current code.
Don't shout retreat conditions. We've always accepted both lower and
upper case conditions. Help has been advertising lower case since
commit bb5dfd8, v4.3.16.
While there, fix a few misspellings and such.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Documented in commit dc41544, v4.3.16. It actually worked only at the
condition prompt then. No longer recognized elsewhere since commit c699949. The documentation is now misleading. Simply drop it; the
prompt points out how to get help,
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
subs: Report where exactly ships and land units sweep mines
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>
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>
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>
navigate: Don't disclose whether unfriendly canal is navigable
When you try to navigate a ship without canal capability into an
unfriendly canal, you get "can't go" when it's below 2%, else "too
large to fit". Always report "can't go" for unfriendly sectors.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
navigate: Require all ships to start in the same sector
The capability to navigate ships spread over several sectors is
obscure and rarely useful. Accidental use is probably more frequent
than intentional use. Issues:
* Interactive prompts show only the flagship's position, and give no
clue that some ships are actually elsewhere.
* Path finding is supported only when all navigating ships are in
the same sector.
* Interdiction becomes rather complex. For each movement, every
sector entered is interdicted independently. This means the same
fort, ship, land unit or plane can interdict multiple times.
Interdiction order depends on the order the code examines
ships. 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 navigate.
I feel we're better off without this feature.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
march: Require all land units to start in the same sector
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>
march: Check for sector abandonment before anyone marches
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>
march: Don't scatter land units on crossing border
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>
When attempting to enter a sector with a ship that can't go there
while the navigating ships are all in the same sector, navigate stops
and prompts without removing the incapable ship from the group. If
another ship has already entered the sector, the group becomes
scattered.
This can happen only when navigating a mixed group of ships with and
without canal capability into a canal. Broken in commit 74e4e281,
v4.3.0.
Remove the incapable ship from the group when another ship can enter
the sector. This avoids scattering ships.
Don't remove incapable ships when no ship can enter the sector.
Without this, navigate would remove everyone and end then.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
* Sail orders are executed at the update. Crafty players can use them
to get around the update window.
* The route is fixed at command time. You can't let the update find
the best route, like it does for distribution.
* The info pages documenting it amount to almost 100 non-blank lines
formatted. They claim you can follow friendly ships. This is
wrong. They also show incorrect follow syntax. Unlikely to be the
only errors.
* Few players use it. Makes it a nice hidey-hole for bugs. Here are
two nice ones:
- If follow's second argument is negative, the code attempts to
follow an uninitialized ship. Could well be a remote hole.
- If ship #1 follows #2 follows #3 follows #2, the update goes into
an infinite loop.
* It's more than 500 lines of rather crufty code nobody wants to
touch. Thanks to a big effort in Empire 2, it shares some code with
the navigation command. It still duplicates other navigation code.
The sharing complicates fixing the bugs demonstrated by
navi-march-test.
Reviewing, fixing and testing this mess isn't worth the opportunity
cost. Remove it instead. Drop commands follow, mquota, sail and
unsail. Drop ship selectors mquota, path, follow.
struct shpstr shrinks some more, on my system from 160 to 120 bytes.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
* Autonavigation orders are executed at the update. Crafty players
can use them to get around the update window.
* Usability is poor:
- The order command is overly complex, not least because it can do
five different things: clear, suspend, resume, declare route, set
cargo levels.
- Unlike every other command involving movement, order does not let
you specify routes, only destination sectors.
- Setting cargo levels can silently swap start and end point of a
circular route, because "this keeps the load_it() procedure
happy". Maybe it does, but it surely keeps players confused.
- Setting "start" cargo levels actually sets the "end" levels, and
vice versa. Has always been broken that way.
- Predicting what exactly autonavigation will do at the update isn't
easy.
* The info pages documenting it amount to almost 400 non-blank lines
formatted. They claim only merchant ships can be given orders.
This is wrong. Unlikely to be the only error.
* Few players use it, and its workings at the update a fairly opaque.
Makes it a nice hidey-hole for bugs. Here are two:
- Unlike the scuttle command, autonavigation happily scuttles trade
ships while they're on the trading block.
- Unlike the load command, autonavigation can load in friendly and
allied sectors.
* It's more than 700 lines of rather crufty code nobody wants to
touch. Thanks to a big effort in Empire 2, it shares code with the
navigation command. It still duplicates load code. The sharing
complicates fixing the bugs demonstrated by navi-march-test.
Reviewing, fixing and testing this mess isn't worth the opportunity
cost. Remove it instead. Drop commands order, qorder and sorder.
Drop ship selectors xstart, xend, ystart, yend, cargostart, cargoend,
amtstart, amtend, autonav.
xdump ship sheds almost half its columns. struct shpstr shrinks, on
my system from 200 to 160 bytes.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
navigate: Fix buffer overrun for impossibly long paths taken
When a player moves more than 1023 sectors in a single navigate
command, we overrun the buffer holding the path taken. Remote hole,
but it requires a ship that can go that far, and even a ship with
speed 1000 would need a tech level well in excess of 1000 for that.
Thus, the hole is purely theoretical for even remotely sane game
configurations.
First known version with the flaw is 4.0.0.
Fix by going back the older behavior: don't print the total path
taken, but do print what the path finder does. Context diff of an
example:
[0:634] Command : nav 3 6,0
Flagship is od oil derrick (#3)
+Using path 'n'
h =
k . .
j d
<67.2:67.2: 6,0> h
od oil derrick (#3) stopped at 6,0
-Path taken: n
This is how march works.
Removes the only use of shp_nav_one_sector()'s unusual return value 2.
Return 1 instead.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
empdump: Omit redundant data from export, new -c includes it
Cuts size of export files in test suite by a factor of four. Not a
big deal for disk usage, as export files compress very well, and disk
space is cheap anyway. Export files are simply easier to work with
when they aren't full of redundant crap.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
The code needs it first so it can store field values into the object
right away. Save the values instead, and store them when the row is
complete.
The improvement is hardly worth the trouble by itself, but it'll
simplify supporting keys consisting of multiple fields, like sector
xloc, yloc or realm cnum, realm, so we can omit rows in those tables,
too.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
xundump: Permit omitting trailing sectors and realms
We currently require all rows to be present for tables item, sect-chr,
infrastructure, sect, realm.
The first three make sense: the code hard-codes indexes for them, and
malfunctions when entries are blank, so we want to make it hard to
leave any blank by accident.
The last two don't: blank sectors and realms work fine. There, the
restriction is arbitrary. Drop it.
Sectors and realms still can't be omitted "in the middle" (can do that
only with an ID selector), but that's coming soon.
Each part of a split table needs to supply rows for the same objects.
We currently require each part to name its objects explicitly, with an
object ID field, and don't support splitting tables that don't have
such IDs. These restrictions became arbitrary when commit 4e23c45
implemented checking each partial table supplies the same rows. Relax
them.
Array selectors were introduced for xdump (commit 612f2da, v4.2.18).
Since there is no syntax for subscripting arrays in conditions and
survey, we silently assume index zero. Unclean. Reject the selector
instead.
Affects ship selectors cargostart, cargoend, amtstart, amtend (since
commit 32011f1, v4.2.19), nat selectors relations (commit 0a44c48,
v4.3.0), contacts (commit c345ab8, v4.3.0), rejects (commit 6844c94,
v4.3.4). The other array selectors aren't visible in conditions. No
array selectors are visible in survey.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
nsc: Rename nstr_exec_val() to nstr_eval() and tighten contract
nstr_exec_val() can produce three different error values: NSC_NOTYPE
on invalid category, invalid type with zero val_as.lng on invalid type
(this is a bug), and the wanted type with zero val_s when it can't
coerce. None of these should ever happen.
Fix it to always produce an NSC_NOTYPE error value. Fix up callers to
check for it.
Specify the result's type is promoted on success. Ensure it is even
when the argument is NSC_VAL with an unpromoted type, which is
invalid.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
tests/files tests/fairland: Check stderr and exit status
New helper run_and_cmp to automate the job: run a program capturing
its standard output, standard error and exit status, then compare the
actual with the expected results.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Feeding other test output files (right now tests/files/files.out
tests/fairland/fairland.out fairland/newcap_script) works, but it's
odd, and might not work as well for all future output files. Only
strip trailing white space there.
There used to be other output files that required normalize.pl: client
output. Gone since commit 9ca3fa9.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
file: Provide EF_WITH_CADEF_MAX_ENTRY_SIZE to clean up xditem()
xditem() needs a buffer that can hold entries of any xdumpable table.
It's been 2048 bytes and marked FIXME since day one. Clean it up so
that if anyone ever goes crazy with entry sizes, we fail an assertion
during startup instead of overrunning the buffer during play.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
player: Drop long-disabled code to resolve IP addresses
Disabled since commit 32fac04 (v4.2.13) because it could at the time
use more stack space than we provided. Additional issues: code still
uses obsolete gethostbyaddr() rather than getnameinfo(), and we
provide only 512 bytes for host names instead of the customary
NI_MAXHOST (1025) bytes.
All three issues would be easy enough to fix. What's not so easy is
to avoid blocking on the synchronous DNS lookup. Without that,
connecting repeatedly from a range of addresses with slow reverse
lookup could conceivably be employed as a denial of service attack.
We've been living without reverse lookup for close to ten years. Bury
the corpse, and move on.
Bonus: sizeof(struct natstr) is cut in half.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
projects / empserver / log