Pull request from copperwater: if a special level's lua file
specified invalid coordinates for some things, the code in sp_lev.c
might use them instead of rejecting them. This could lead to severe
problems. Presumeably the existing special levels aren't affected
by this.
Closes#1034
Revealed this bug when testing the previous commit:
Themed room generation with a randomly placed map involves picking a
single random point on the map at which to plop it down, and then
declaring the themed room failed and exiting if it would go beyond the
map bounds or overlaps with an existing room. In the process,
xstart/ystart/xsize/ysize have been modified, but weren't getting reset.
(They would get reset if the map successfully got placed and it had a
contents function, as of commit 4af086b, but there wasn't handling for
the failure to place it.)
I traced a memory corruption bug in xNetHack to a themed room that
looked something like this:
function()
des.room({ type="themed", contents = function()
des.feature({ type='sink' })
...
end })
end
Placing a feature at a random spot within a room or region is a
reasonable thing for the parser to handle, but the code was not equipped
to handle it, and so the unspecified x and y set as -1 got passed
directly to SP_COORD_PACK, ending up as coordinates way off the map.
Since sel_set_feature does not do an isok() check, this ended up writing
data to unrelated memory.
This commit does the following things:
- Enables des.feature() with no coordinates specified, both via a table
with 'type' set, and as the single string argument. When no
coordinates are specified, it will pick a random normal-floor spot
within the enclosing room or region if there is one, or anywhere
on the level if there isn't.
- Prevents sel_set_feature from corrupting memory outside
g.level.locations. Additionally, if EXTRA_SANITY_CHECKS is defined and
this gets attempted, it causes an impossible.
- Guards the existing "door coord not ok" Lua error with an immediate
return from lspo_door.
- Adds similar "coord not ok" errors to all the other locations in
sp_lev.c which did not already check for a unspecified/invalid
coordinate and for which a random coordinate is nonsensical:
des.terrain(), des.drawbridge(), and des.mazewalk().
The FIXME comment noted that builds_up would return an incorrect false
value for a dungeon branch that builds upwards but is only 1 level, but
that this is a latent problem because no such branch exists in NetHack.
Such a branch does exist in xNetHack, and it causes the debug fuzzer to
crash ("mon_arrive: no corresponding portal" because it can't find the
correct-direction stairs), so I figured I might as well fix it upstream.
Pull request from copperwater: accept 'true' or 'false' (also 'yes'
or 'no') in des.object specification for 'trapped'.
After resolving a merge conflict one diff band of this went away, but
the settings of otmp->otrapped from o->trapped and otmp->greased from
o->greased without it look ok to me.
Closes#1032
I thought there were more object fields that currently only accept ints
but ought to accept booleans, but when I checked I found that most of
them do already, and the ones that take ints are the ones that the
number carries meaning (spe, recharged, etc).
Except for trapped. In struct obj, otrapped is a 1-bit flag, so there's no
good reason for the level parser to treat it as a type error if someone
intuitively makes a des.object call with trapped=true or trapped=false.
Change it to an optional boolean argument, like the other boolean flags
(locked, greased, etc).
Note that get_table_boolean_opt still accepts ints, so existing uses of
trapped=0 or trapped=1 won't be affected.
Something that's reasonable to expect to see in Lua files is something
like:
local sel4 = sel1 - sel2 - sel3
or more generally, producing a selection from subtraction that will then
be used in subsequent selection math.
I discovered this wasn't actually working correctly, and that it also
applied to the xor operation. The reason behind this is that
l_selection_sub and l_selection_xor create a new selection from nothing,
which by default has "lower" bounds of COLNO, ROWNO and "upper" bounds
of 0,0. Iterating across the intersecting rectangle of both selections
does not reliably set the bounds of the resulting selection properly,
since the first selection_setpoint with a value of 0 will cause the
selection's bounds_dirty flag to be set, at which point they will cease
to change as more points are added.
Then this selection with its incorrect boundaries is pushed back onto
the Lua stack, and becomes the first operand of the next subtraction
(i.e. selr in the first l_selection_sub becomes sela in the second
l_selection_sub). Depending on how broken the bounding box is, results
may vary, but if the bounding box is still (COLNO,ROWNO,0,0), the
resulting selection will have no points selected at all.
This fixes this problem by forcibly recalculating the bounds of the
result selection, so any subsequent operations on it will be valid.
I was looking to see what the impact of converting glyph_to_cmap() to
a function might have and noticed a couple of places where the macro
edition is passed a non-trivial argument. Since it evaluates that
argument many times, there is hidden overhead. Fetch the glyph once
and pass that to glyph_to_cmap().
This will no longer be very useful--but won't need to be reverted--if
PR #1022 gets incorporated.
do_positionbar() has bugs (only matters for MS-DOS).
Pull request from NulCGT: make statues created for statue traps be
5 to 10 points higher in difficulty than the default would be.
5 to 10 points of difficulty higher is already used for figurines.
The pull request chose the same amount but I've reduced it to 3 to 6.
Partly so that they won't be the same, partly so that they won't be
too hard when activated, and partly so that the creature won't be
quite as obvious a give away that the statue is a trap.
Closes#1009
Pull request from NulCGT: when a doppelganger is choosing to become
a fake player, get role from an entry in the high scores file. Use
that entry's name too if the doppelganger is within view at the time.
I'm not sure how well this will work for a single user score file if
the player always runs the same role and name. I've given it a small
chance (1/13) to ignore the topten and stay with random role instead.
Closes#1008
I saw this in the YANI archive, and I think it's fairly interesting.
Doppelgangers are known for commiting identity theft, but in NetHack
they function as just another shapeshifter. This commit makes them
a bit more interesting, I think.
Original YANI by aosdict and Andrio.
Pull request #1005 introduced some code that needed minor reformatting.
This goes beyond that, splitting the livelog/#chronicle handling and
the vampire unshifting code out of mondead() into separate routines.
The livelog phrasing for death of a unique monster always assumed that
the hero was directly responsible:
"killed|destroyed <unique mon|shk+details> (Nth time)."
If it dies while monsters are moving, switch to:
"<unique mon|shk+details> has been killed|destroyed (Nth time)."
It isn't phrased as if the hero is responsible if the monster dies due
to passive counter-attack against poly'd hero but that isn't blatantly
wrong and doesn't seem worth worrying about.
The livelog message includes the shopkeeper's shop type, since the
livelog for stealing also mentions the shop type.
Only the first kill per shopkeeper is logged.
The command name is expected to be short and the menu description is
expected to be short, but use a bigger buffer for the combination of
the two just in case.
In the paranoid_confirm submenu for 'm O', dynamically substitute the
correct key into the description of the 'swim' choice if #reqmenu is
bound to something other than 'm'.
Issue reported by ostrosablin (and mentioned previously but I still
I haven't remembered where): loading a roguesymset removes any utf8
data that has been set up for primary symset. The curses interface
explicitly initializes roguesymset to the default set and if config
file has specified OPTIONS=symset:Enhanced1 (or some other uft8 set
if someone adds such), that stays the active set but no longer gets
rendered with the intended symbols.
I have no idea whether having symset and roguesymset both use uft8
with different symbols and/or colors works at all and if so whether
it will still work after this revision, but this prevents loading one
set with non-utf8 while the other still uses utf8 from clearing out
the cached utf8 data.
Closes#1026
This should pacify the analyzer. get_artifact(obj) expands to code
which checks whether obj is Null. untouchable() knows that it will
always be non-Null so didn't perform any comparable test and the
analyzer complained.
Using get_artifact() before the 'beingworn' assignment instead of
after would just have shifted the complaint to the assignment's use
of obj->owornmask. Adding a test for Null to that expression should
eliminate the complaint but I haven't verified that.
Player reported that a cat which was asleep in a treasure zoo would
catch and eat thrown food, becoming tamed in the process, without
waking up.
Temporary sleep was handled (via checking for !mon->mcanmove) but
indefinite sleep wasn't. Make a monster hit by taming effect (either
magic or thrown food) wake up from indefinite sleep. For temporary
sleep, the remaining duration is cut in half. The timeout isn't
dropped all the way to 0 because a monster with mon->mfrozen set
isn't necessarily asleep. That timeout is shared by timed sleep,
timed paralysis, and busy time when donning armor.
Introduce
OPTIONS=paranoid_confirm:+foo bar
to add the 'foo' and 'bar' bits to any confirmation bits already set and
OPTIONS=paranoid_confirm:-foo bar
to clear the 'foo' and 'bar' bits without changing any others that are
already set. Values '+foo +bar' or '+foo -bar' or '!foo bar' aren't
accepted.
OPTIONS=paranoid_confirm:foo bar
still replaces existing settings with just the specified bits.
OPTIONS=!paranoid_confirm:foo bar
is treated like 'paranoid_confirm:-foo bar', although I'm wondering
whether that was good idea. Negation combined with plus or minus is
rejected.
Replace the handling for 'prayconfirm' to use 'paranoid_confirm:+pray'
and '!prayconfirm' with 'paranoid_confirm:-pray'. Issue a warning
(which is described as "error" but it still works) about it being
deprecated when that out of date boolean is used.
Reorder swim and AutoAll in the paranoid_confirm menu so that the ones
that add a new 'y' question (pray and AutoAll) are together and the
two that change game play (swim and Remove) without asking questions
are together. (The description of swim in its menu entry now mentions
the 'm' prefix and should probably plug in the value instead since it
might be bound to some other key.)
Undefine the macros defined for option.c's use prior to end of file.
add paranoid_confirmation:AutoAll
update paranoid_confirmation:pray (for paranoid_confirm:Confirm)
update paranoid_confirmation:swim (reorder, and on by default)
update menustyle:full (for paranoid_confirm:AutoAll)
Guidebook.tex is updated but not tested.
An issue from nearly three years ago, reported by Anerag: asking
player whether to really pray isn't paranoid enough because it
accepts 'y' rather than requring "yes".
This changes it to require "yes" followed by <return> or <enter> if
paranoid_confirm:Confirm is also set. (A side-effect of that is
explicit "no<return|enter>" is required instead of just 'n' to
decline--for all the paranoid confirmations, not just for prayer.)
This extension of paranoid:Confirm applies to paranoid:AutoAll too.
A comment asks why paranoid_confirm:pray is different from the other
paranoid questions in the first place. The answer is that when it
isn't set, no confirmation prompt is issued at all. The others all
have y/n confirmation prompts when the corresponding paranoid option
isn't set.
Once upon a time there was a boolean option called 'prayconfirm' that
issued "really pray?" prompt when True. It was added after players
whinged about typing Alt+p when they meant to type Alt+o. When the
more advanced 'paranoid_confirmation' was introduced, paranoid:pray
superseded prayconfirm, but it still only issues a confirmation
prompt where there normally wouldn't be one rather than change an
existing one to require "yes<return|enter>" instead of 'y'.
Closes#303
If confirmation for menustyle:Full 'A' choice is on and player
chooses 'A' but answers no at the "Really autoselect All?" prompt,
just remove 'A' from the list of choices and act on the rest. If it
was the only choice, the menu operation will behave as if no choices
had been made. (That was the previous behavior even when other
choices were also present.) But when other choices have been made
along with 'A', use them without 'A': the normal second menu will
be put up, listing objects matching the specified classes and not
autoselecting anything.
Issue most recently reported by Xdminsy (previously reported by
others): it is too easy to accidentally pick choice 'A' in object
class selection menus for menustyle:Full. Previous change relevant
to this was to exclude choices 'A' and 'a' from being set by '.'
(choose all entries) and '~' (toggle all entries). That was an
improvement but doesn't help with pressing shift when meaning to
type 'a' by those who type faster than they cogitate.
This implements a suggestion by janne-hmp: add new choice for the
paranoid_confirmation option, 'Autoall' (synonym 'Autoselect-all').
If the player sets this and includes 'A' among the choices for
class selection, prompt to confirm whether to honor it. Like
confirmation for praying, it adds an extra y/n prompt rather than
change an existing y/n prompt into a yes/n or yes/no one. If the
player declines, then nothing is selected and the operation is
cancelled rather than putting the menu back up to choose again.
OPTIONS=paranoid_confirm:autoall requires at least two letters
('au') even if the 'a' is capitalized. paranoid_confirm:a means
confirm attacking peaceful monsters. And it should be
OPTIONS=paranoid_confirm:autoall pray swim
if someone just wants to add autoall to the default paranoid bits.
The Guidebook hasn't been updated to describe the new choice since
it seems likely that it might undergo adjustments.
Closes#1065
While running the tutorial, the Save command is disabled. When the
tutorial was extended to two levels, stashing and restoring the
hero's equipment stopped working as intended if player entered the
second level. The attempted fix for that broke re-enabling Save
even if the player left the tutorial without entering its second
level.
This seems to fix things, but I'm flailing around with barely a clue
here. A couple of simpler attempts didn't work and I haven't figured
out why, so this is a bit more complex than what I wanted.
Reorganizing nhl_callback() isn't part of the fix, just avoids use
of some redundant code.
Update dlb_main to use the revised vms_basename() for #if VMS in
case it issues a usage message.
Change recover to use similar setup, but the vms-specific bit is
commented out because I'm not sure whether the necessary routine
is being linked with it.
Avoid a conflict with c++ std header file on at least one platform.
Build log prior:
In file included from DKA100:[DEVEL.nethack-37.sys.share]cppregex.cpp;1:12:
In file included from /SYS$COMMON/VSICXX$LIB/INCLUDE/LIB_CXX/INCLUDE/regex:768:
In file included from /SYS$COMMON/VSICXX$LIB/INCLUDE/LIB_CXX/INCLUDE/stdexcept:5
1:
In file included from /SYS$COMMON/VSICXX$LIB/INCLUDE/LIB_CXX/INCLUDE/exception:8
7:
In file included from /SYS$COMMON/VSICXX$LIB/INCLUDE/LIB_CXX/INCLUDE/cstdlib:91:
In file included from /SYS$COMMON/VSICXX$LIB/INCLUDE/LIB_CXX/INCLUDE/stdlib.h:10
3:
/SYS$COMMON/VSICXX$LIB/INCLUDE/DECC$RTLDEF/stdlib.h:200:24: error: too many argu
ments provided to function-like macro invocation
void abort (void);
^
../INCLUDE/vmsconf.h:307:9: note: macro 'abort' defined here
#define abort() vms_abort() /* vmsmisc.c */
^
vms_basename() was recently changed to take a second argument to
control whether to include the suffix portion of the name but an
existing call to set up 'hname' still had only one.
More than just adding the extra argument was needed. It returns
a static buffer so if it got called for DEBUGFILES, 'hname' would
have been clobbered.
vms_basename() was recently changed to take a second argument to
control whether to include the suffix portion of the name (used for
DEBUGFILES) but an existing call still had only one.
Use of select-all, select-page, toggle-all, and toggle-page in the
object class selection menu for menustyle:full excluded A (auto-
select all), a (all classes), B, C, U, X (bless/curse states), and
P (recently picked up) but allowed u and x (unpaid and used-up shop
goods) to be set. Treat u and x like the other pseudo-classes;
they have to be chosen explicitly rather than be set with . and ~.
gcc-13.1 static analyzer complains that alloc() returns long *
without guaranteeing to allocate an integral number of longs. Fix
by rounding the requested amount up to the next long when dividing
the amount by 'sizeof (long)' yields a remainder. Surprisingly--to
me, at least--the analyzer recognizes that this extra argument
manipulation will always produce a viable amount no matter what
alloc()'s caller passes in.
Also, the declarations for alloc() and re_alloc() in alloc.c didn't
match the ones in global.h for the MONITOR_HEAP config. I guess
nobody has tested that since NONNULL got introduced.
A year ago the two FITSxxx routines were moved from hacklib.c to
alloc.c so that they could easily be linked into various programs
instead of being replicated in each, but the declarations for them
weren't moved from hacklib.c section in extern.h to alloc.c one.
