The autonavigation feature has issues:
* 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>
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>
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>
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>
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>
We're dealing with three kinds of string storage: char * pointing to a
null-terminated string, char[] holding a null-terminated string, and
char holding a string of length 0 or 1.
Unfortunately, xdump meta data doesn't distinguish the latter two:
both are NSC_STRINGY. Because of that, xundump happily fills char[]
to the limit, producing strings that aren't null-terminated, resulting
in read beyond buffer and possibly worse.
Affects struct shpstr members shp_path, shp_name, shp_rpath, struct
lndstr member lnd_rpath, and struct natstr members nat_cnam, nat_pnam,
nat_hostaddr, nat_hostname, nat_userid. Since these are all in game
state, only the empdump utility program is affected, not the
configuration table reader.
We clearly need to require null-termination for char[] values. Since
using char[1] for null-terminated strings makes no sense, we can still
make NSC_STRINGY with length 1 serve char values as before, by
permitting non-null-terminated strings only when length is 1. Ugly
wart, but it fixes the bug without a possibly awkward change xdump
meta-data.
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>
Members x, y, eff, supplied are not "LAND only", they're used only by
ground combat. They're not used by march.
In ground combat, member mobil is not how much the land unit has left,
it's how much it still needs to be charged.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Some callers have to second-guess shp_check_nav() to figure out
whether CN_LANDLOCKED means "too big to fit into the canal" or "can't
go there at all".
Fix that by returning d_navigation. CN_LANDLOCKED becomes either
NAV_CANAL or NAV_NONE, CN_CONSTRUCTION becomes either NAV_02 or
NAV_60, and CN_NAVIGABLE becomes NAVOK.
The CN_NAVIGABLE, ... codes are now unused. Drop them.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
TREATIES has issues:
* Treaties can cover attack, assault, paradrop, board, lboard, fire,
build (s|p|l|n) and enlist, but not bomb, launch, torpedo and
enlistment centers.
* Usability is very poor. While a treaty is in effect, every player
action that violates a treaty condition triggers a prompt like this:
This action is in contravention of treaty #0 (with Curmudgeon)
Do you wish to go ahead anyway? [yn]
If you decline, the action is not executed. If you accept, it is.
In both cases, your decision is reported in the news.
You cannot get rid of these prompts until the treaty expires.
* Virtually nobody uses them.
* Virtually unused code is buggy code. There is at least one race
condition: multifire() reads the firing sector, ship or land unit
before the treaty prompt, and writes it back after, triggering a
generation oops. Any updates made by other threads while trechk()
waits for input are wiped out, triggering a seqno mismatch oops.
* The treaty prompts could confuse smart clients that aren't prepared
for them. WinACE isn't, but is reported to work anyway at least
common usage. Ron Koenderink (the WinACE maintainer) suspects there
could be a few situations where it will fail.
This feature is not earning its keep. Remove it. Drop command
treaty, consider treaty, offer treaty, xdump treaty, reject treaties.
Output of accept changed, obviously.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Adding or removing a command to/from a test has unfortunate effects:
* Before the previous commit: if the command consumes pseudorandom
numbers, all subsequent users of pseudorandom numbers get different
ones. This has always been a major headache.
* Since the previous commit: all subsequent users of pseudorandom
numbers get different ones whether the command consumes any or not.
That's even worse.
* If the command uses BTUs, subsequent prompts are changed. Not
nearly as bad as the above, but still annoying.
Create a new command __cmd to allow compensating for adding/removing
commands for tests. Throw in the ability to compensate treasury
changes for good measure. Three arguments: command count, BTU use,
money use.
Usage example: say you add a convert command to a test, and it uses 3
BTUs and $15. Then you compensate by adding "__cmd added 1 3 15"
right after it.
The command must not be available unless running_test_suite is on, of
course. Make it require the new player command capability TESTING,
and give that to all players when running_test_suite is on.
The command is intentionally not documented in info. Switch
running_test_suite off for info-test, to hide it (and any future
TESTING commands) from info-test.
Suppress the command counter increment for TESTING commands, so they
can be used without upsetting pseudorandom numbers
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
For now, it just logs "Configured for testing" on startup, and prints
a scary warning on player login.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Print a message, send bulletin to owner. Affects ship key 'W', land
unit key 'W', and plane key 'f'. The message is necessary to give the
deity a chance to catch unexpected changes, e.g. a player modifying
retreat conditions right before the deity edits them. Watching out
for such changes is especially important with non-interactive edit.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Edit lets deities load units onto remote carriers, resulting in a
carriers having cargo in another sector. Not good. Cargo gets
teleported to its carrier belatedly when the carrier moves.
Better let edit take care of the teleport.
Also tell the deity that he just caused a teleport. Necessary to give
the deity a chance to catch unexpected changes, e.g. a player moving a
plane right before the deity edits it. Watching out for such changes
is especially important with non-interactive edit.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Print a message, send bulletin to owner. Affects plane keys 's', 'y',
and land unit keys 'S', 'Y'. The message is necessary to give the
deity a chance to catch unexpected changes, e.g. a player loading a
plane right before the deity edits it. Watching out for such changes
is especially important with non-interactive edit.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Print "unchanged" instead of "changed from X to X". Affects edit,
setresource, setsector. Suppress bulletin and news. Affects only
edit sector key 'L'.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Send bulletin to owner and report news exactly like for key 'o'.
Also print a "sector duplicated" message, just for consistency with
other keys.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
Commit 77e95bd7 (v4.3.12) missed the move of log.c there. A few
pointers to other headers are missing.
Signed-off-by: Markus Armbruster <armbru@pond.sub.org>
