Issue reported by ars3niy: an arrow trap covered by more than
one stack of arrows was described by farlook as "unexplored area".
Later simplified to any object pile with plain arrows on top; the
trap turned out to be irrelevant aside from producing a pile of
arrow stacks.
This problem turns out to be due to an off by 1 error introduced
into the xxx_is_piletop() macros when generic objects were added in
mid-January of last year, and plain arrow is the first real object
so suffered from the bug. Misclassifying the glyph at a specific
location also confused the #terrain command but it's the same bug.
Knowing what was wrong (map glyph was UNEXPLORED when it should
have been ARROW) wasn't sufficient to figure out the problem.
Without the simplified way of reproducing the problem, I would
never have managed to use 'git bisect' to find the offending commit
because that would have been too time consuming.
Fixes#1289
Take care of most of include/*.h. I punted on extern.h.
For both src/*.c and include/*.h, I used mismatched checks of
width > 79 to decide which files to look at and then width > 78
to decide which lines to maybe revise, so I didn't look at a bunch
of the files.
I don't plan to go back and do it right. Shortening lines that are
80 or wider to less than 80 is the significant part. Otherwise
emacs puts a backslash in column 80 and the rest of the line of text
on the next line of the screen, making things harder to read.
This makes custom S_sw_tc etc. from Enhanced1 symset actually work,
yielding nice smooth outlines for swallowers and explosions. Or so I
think, I have only tested the former because when playing locally,
explosions disappear so fast I cannot see them.
While looking at where else glyph_to_swallow was used, I noticed that
parse_id subtracted S_sw_tl from glyph_to_swallow, even though it
returns 0 to 7. This looks like it would cause out-of-bounds access and
perhaps a segfault when trying to customise glyphs for individual
monster swallow glyphs (or whatever it is parse_id is used for), but
I haven't tried to confirm nor change this because who would ever do
such thing?
Even if nethack is meant to support compilers that do not know about
inline functions, this one was frankly too long to be a macro already,
and I'm about to make it longer still.
The g? structs had a mix of variables that were written to
the savefile, and those that were not.
For better clarity and to distinguish those that end up in
the savefile, relocate some g? variables that get written
directly to the savefile into different structs.
This updates EDITLEVEL, although technically it probably
didn't need to, since savefile contents are not changing.
Details:
gb.bases -> svb.bases
gb.bbubbles -> svb.bbubbles
gb.branches -> svb.branches
gc.context -> svc.context
gd.disco -> svd.disco
gd.dndest -> svd.dndest
gd.doors -> svd.doors
gd.doors_alloc -> svd.doors_alloc
gd.dungeon_topology -> svd.dungeon_topology
gd.dungeons -> svd.dungeons
ge.exclusion_zones -> sve.exclusion_zones
gh.hackpid -> svh.hackpid
gi.inv_pos -> svi.inv_pos
gk.killer -> svk.killer
gl.lastseentyp -> svl.lastseentyp
gl.level -> svl.level
gl.level_info -> svl.level_info
gm.mapseenchn -> svm.mapseenchn
gm.moves -> svm.moves
gm.mvitals -> svm.mvitals
gn.n_dgns -> svn.n_dgns
gn.n_regions -> svn.n_regions
gn.nroom -> svn.nroom
go.oracle_cnt -> svo.oracle_cnt
gp.pl_character -> svp.pl_character
gp.pl_fruit -> svp.pl_fruit
gp.plname -> svp.plname
gp.program_state -> svp.program_state
gq.quest_status -> svq.quest_status
gr.rooms -> svr.rooms
gs.sp_levchn -> svs.sp_levchn
gs.spl_book -> svs.spl_book
gt.timer_id -> svt.timer_id
gt.tune -> svt.tune
gu.updest -> svu.updest
gx.xmax -> svx.xmax
gx.xmin -> svx.xmin
gy.ymax -> svy.ymax
gy.ymin -> svy.ymin
Related note:
There are some pointer variables that are heads of chains that were not
moved from 'g?' to 'sv?', because they are not actually written to the
savefile directly, but the objects/monst/trap/lightsource/timer in the
chains they point to are. That can be changed, if desired.
Examples: gi.invent, gm.migrating_objs, gb.billobjs, gm.migrating_mons,
gf.ftrap, gl.light_base, gt.timer_base
Checking the callers:
newsym() the use of see_with_infrared() is guarded by
} else if ((mon = m_at(x, y)) != 0 [...]
do_mgivenname() the use of see_with_infrared is guarded by !mtmp:
&& (!mtmp
|| (!sensemon(mtmp)
&& (!(cansee(cx, cy) || see_with_infrared(mtmp))
howmonseen(mon) dereferences mon in other places, so it would
segfault if mon were NULL; howmonseen has NONNULLARG1.
callers were checked:
domove_attackmon_at(mtmp, x, y, displaceu) has mtmp declared nonnull;
there are dereferences of mtmp in the first line of code in
the function.
In domove_core():
The 1st occurrence of is_safemon(mtmp) is guarded by if (mtmp) { }.
The 2nd occurrence of is_safemon(mtmp) is inside an if (mtmp) { } block.
The 3rd occurrence of is_safemon(mtmp) was just remediated by 987be7e8.
In lookaround():
The only occurrence of is_safemon(mtmp) is inside an
if ((mtmp = m_at(x, y)) != 0 [...] { } block.
In do_attack(mtmp), in uhitm.c:
The parameter is declared NONNULLARG1, and the 1st line of
code contains a dereference with mtmp->data, which would
segfault if mtmp were NULL.
This fixes the part of pull request #1102 by entrez dealing with the
map refresh side of things. It was pulled out of a much larger patch
that also deals with terminal window resize for tty.
Using ^R when getpos() is in operation, whether actually picking a
position for something or browsing the map during #terrain or post
detection magic, it was reconstructing the known map and positioning
the cursor on the hero instead redrawing the selected terrain subset
or detected objects/monsters/whatever. There's already a routine to
redraw the current view of the map without recalculating it, but it
wasn't being used for ^R during getpos operation.
The default engraving-in-corridor character is the same as the default
corridor symbol (and also default lit corridor one), distinguished by
color. Show it differently (in inverse vidoe, like lava vs water and
sink vs fountain) if color is Off.
It might be better to change the engraving-in-room symbol to be the
same as the room one so that they'll be more consistent with corridors;
color is probably sufficient without resorting to back-tick. But this
update hasn't done that.
1. Add "engraved room floor" pchar sym (S_engroom). The symbol that
displays at the engraved part of a room (not a corridor though).
The default symbol is '`' which is currently never shown if people
have defined the boulder symbol to '0' and statues are displayed as
monster symbols. It is bright blue.
Add some stylized variations of the S_engroom symset to some of
the symsets.
2. Add "engraved corridor" pchar sym (S_engrcorr). The symbol that
displays at the engraved part of a corridor. The default symbol is
'#', and it matches the symbol for corridor from for whatever the
current symset uses. It is bright blue to match the color of the
S_engroom symbol. Using the normal corridor symbol for display
preserves the lines of the corridor so is not as visually-disruptive
as a smaller symbol would be. Explicit entries that match the S_corr
symbol have been added to the symset file.
Magic mapping and clairvoyance impacts yet to be determined.
The Guidebook updates will come later.
When hallucinating, random object selection for objects was including
the new generic objects. It was already excluding 'strange object'
by using 'rn2(NUM_OBJECTS - 1) + 1' to skip objects[0]; changing that
to be 'rn2(NUM_OBJECTS - MAXOCLASSES) + MAXOCLASSES' will skip the
first 18 objects, 'strange object' plus the 17 generic objects.
(I'm trying to convince myself that there's no off-by-1 or off-by-N
error and think I've succeeded.)
Try to fix a fuzzer issue. I wasn't able to reproduce it so am not
sure whether this actually fixes it. A mimic seemed to be mimicking
object #1 (generic ILLOBJ_CLASS object which shouldn't occur) rather
than #0 (strange object). Strange object always has dknown==1 and
generic objects should always have dknown==0 but farlook of mystery
object #1 had its dknown flag set.
An earlier fix to force non-Null oc_name when formatting objects in
order to pacify the static analyzer might have been the reason that
the problem couldn't be reproduced.
This includes a few miscellaneous changes made while unsuccessfully
hunting for the problem.
Add 17 fake objects to objects[], one for each object class. All
specific color as gray. They're grouped at the start--actually near
the start since "strange object" is still objects[0]--rather than
being among the objects for each class. init_object() knows to start
at [MAXOCLASSES] instead of [0]; other code that loops through every
object might need adjusting.
For potions, non-stone gems, and non-novel/non-Book_of_the_Dead
spellbooks that don't have obj->dknown set, display the corresponding
generic object rather the object itself. Fixes the longstanding bug
of seeing color for not-yet-seen objects whose primary distinguishing
characteristic is their color. Walking next to a generic object
while able to see its spot will set dknown and redraw as specific.
It's slightly disconcerting to have objects change as you reach them;
I hope it's just a matter of becoming used to that. (If there is any
code still changing the hero's location manually instead of using
u_on_newpos(), it should be changed to use that routine.)
Most of the new tiles are just a big rendering of punctuation
characters. The potion, gem, and spellbook ones could be cloned from
a specific object in their class and then have the color removed. I
started out that way but wasn't happy with the result. I'm not
artisticly inclined; hopefully someone else will do better. Each of
them is preceded by a comment beginning with "#_"; the underscore
isn't required, just being used to make the comments stand out a bit.
Invalidates existing save and bones files.
Also includes support by paxed for polearm targeting using the
frame color.
Also renames USE_TILES to TILES_IN_GLYPHMAP which is a more
accurate description.
Not all window interfaces have full support for the color framing
of the background square yet.
MS-DOS needs further work (to bring it to both VESA and VGA, with
and without tiles.
Windows GUI is missing support.
X11 and Qt have been started, but may require further refinement.
The consolidation of global variables from scattered source
files into decl.c and declared in decl.h was begun in 3.7.0.
Their placement in common files was done for centralized
initialization and potential re-initialization during a
"play again" scenario.
It wasn't really necessary for all of them to be housed in a
single huge structure to meet the "play again" requirement,
and the single huge structure has been a little unwieldy when
it comes to maintenance.
Following this commit, instead of one single extremely large structure
named 'g' to house all of the relocated global variables, they
are distributed into several ga through gz.
To make things easy for the developer, each variable is placed
into the struct corresponding to the starting letter of the variable.
That way, no lookup is required in order to know which struct houses
a particular variable, it is a simple match to the starting letter
for all the centralized global variables.
A global variable named 'amulets', would be found in ga.
ga.amulets
^ ^
A global varable named 'move', would be found in gm.
gm.moves
^ ^
A global variable named 'val_for_n_or_more' would be found in gv.
gv.val_for_n_or_more
^ ^
A global variable named 'youmonst' would be found in gy.
gy.youmonst
^ ^
Short for distu(mtmp->mx, mtmp->my) (i.e. the distance between the hero
and the specified monster), which is a very common use of distu(). The
idea is that this would be a convenient shorthand for it; I actually
thought it (or something very similar) existed already, but couldn't
find it when I tried to use it earlier. Based on the number of uses of
fully-spelled-out 'distu(mtmp->mx, mtmp->my)' replaced in this commit
I'm guessing I just imagined it.
The pull request included some changes that were neither accidental nor
unintentional, so only a subset of the changes from pull request #869
submitted by klorpa were manually applied.
behaviour -> behavior
speach -> speech
knowlege -> knowledge
incrments -> increments
stethscope -> stethoscope
staiway -> stairway
arifact -> artifact
extracing -> extracting
The uses of "iff" were left alone.
Close#869
Some static analyzers flagged the last-resort values as
out of bounds (which they were).
There's a small number of other complaint-suppression items in here too,
but nothing drastic.
Some discrepencies between glyph_is_cmap and glyph_to_cmap
arose after b14b830b because the change resulted in
glyph_is_cmap matching on zap beams which weren't accounted
for in the glyph_to_cmap macro. It is unlikely that
glyph_to_cmap will ever be used on such a glyph, but at least
have glyph_to_cmap return a sane value rather than drop through
to the last-resort value (currently NO_GLYPH) which is far
outside the range of the defsyms[] array indices.
I think some of these discrepencies between glyph_is_cmap and
glyph_to_cmap started after b14b830b because the additional
ranges added by that didn't have a corresponding return in
glyph_to_cmap.
When moat and lava use the same screen symbol and color is Off, lava
is rendered in inverse video. It used to be similar for floor and
ice, but that got broken last year. Fix inverse ice, and now that
fountain and sink might be same symbol (recent IBMgraphics change),
render sinks in inverse if they match fountains and color is Off.
I started to give sink its own mapglyph flag but then got lazy and
used the same value as ice. That can be amended if some interface
wants to use some more elaborate distinction than inverse video.
Change some more
(first_line <op> \
second_line)
to
(first_line \
<op> second_line)
for various values of <op> besides '&&' and '||'. Also clean up some
indentation and backslash+newline alignment.
For object_is_piletop() don't bother to validate that an object which
is classified as being on the floor is actually on the floor at its
specified map coordinates. That check was propagating to expansions
of multiple macros and if it ever failed it would just be hiding a
very serious bug without helping to fix that.
I captured preprocessor output while debugging vault guard changes
and noticed that glyhp_is_cmap() was expanding to an excessive amount
of code. Simplify it.
Also, a bunch of the cmap macro definitions were using old
(foo && \
bar)
instead of current
(foo \
&& bar)
so this changes those.
One of the drivers of this change was that screen coordinates require a
type that can hold values greater than 127. Parameters to the window
port routines require a large type in order to be able to have values
a fair bit larger than COLNO and ROWNO passed to them, particularly for
their use to the right of the map window.
This splits the uses of xchar into 3 different situations, and adjusts
their type and size:
xchar
|
-----------------------
| | |
coordxy xint16 xint8
coordxy: Actual x or y coordinates for various things (moved to 16-bits).
xint16: Same data size as coordxy, but for non-coordinate use (16-bits).
xint8: There are only a few use cases initially, where it was very
plain to see that the variable could remain as 8-bits, rather
than be bumped to 16-bits. There are probably more such cases
that could be changed after additional review.
Note: This first changed all xchar variables to coordxy. Some were
reviewed and got changed to xint16 or xint8 when it became apparent that
their usage was not for coordinates.
This increments EDITLEVEL in patchlevel.h
For the tty perm_invent boundary box initialiation, instead of doing
one of many assignments that do glyph lookup, do one of many symbol
assignments and one glyph lookup. No change in observable behavior.
Also, use the main dungeon's walls for box rendering instead of
selecting ones for whatever branch the hero happens to be in at the
time perm_invent gets enabled.
I wasted a bunch of time yesterday trying to figure out why a maze
level in Gehennom wasn't being shown with orange walls and ended up
reformatting a few glyph handling macros while hunting for the problem.
It turned out that the wall color choosing was working as intended.
I was looking at a maze bordered by solid stone (pale blue with Qt's
tiles, unlike tty's blank space) rather than by walls.
Anyway, a couple of the macros have had a little bit of common code
factored out rather than just be reformatted so I'm putting this in.
[For future, maybe stone should be given branch-specific coloring
similar to walls?]
I recently captured preprocessor output for a file and the amount of
code being expanded--and subsequently compiled--for canspotmon() was
quite an eye opener. This converts most of the macros it uses into
function calls. The resulting executable generated for OSX (built
for x86_64 and containing four interfaces) is about 5.5% smaller! and
there wasn't any difference in speed that I could notice.
The knowninvisible() macro has been in error for as far back as the
git logs go (which include those for the second cvs repository, so
over 20 years now).
High altars and normal temple altars had identical altarmasks, so
there was no way to distinguish between the two based on the altarmask
alone. Instead, anywhere it was necessary to determine whether an altar
was a high altar included a check whether the hero was currently the
Astral Plane or Moloch's Sanctum, and assumed any temple altar was the
high altar.
Since there's an extra, unused bit in altarmask anyway, use it to
explicitly mark high altars -- the lua level files already distinguish
between normal temple altars and so-called 'sanctum' altars anyway, so
rather than throwing away this distinction when generating the level,
keep it in the altarmask and use it in place of various u.uz checks.
I think this would require incrementing EDITLEVEL again...
"add glyphs+tiles for door+chest traps",
commit d1217b9f25,
missed a couple of S_vibrating_square references, resulting in
screwed up rendering of zaps and explosions.
When trap detection finds trapped doors and trapped chests, it shows
those as bear traps. When the hero comes within view, they revert to
normal and the detected trap is forgotten. This doesn't change that,
it is just groundwork to be able to show them distinctly. Like the
TT_BEARTRAP patch, it increments EDITLEVEL so this seemed like a good
time to put the groudwork in place.
There shouldn't be any visible changes even though internal glyph and
tile values have been renumbered after inserting two new entries.
Adding traps after S_vibrating_square was quite a hassle and suffered
though a couple of off-by-one errors that weren't trivial to find and
fix.
There are no longer distinct gendered versions of monsters, so femalenum
is unused (i.e. set to NON_PM) for all roles and races. Take a pass at
removing all uses of/references to femalenum, and rename 'malenum' to
'mnum' since it no longer has any particular association with
gender or sex.
In file included from ../include/hack.h:217,
from ../win/Qt/qt_plsel.cpp:18:
../win/Qt/qt_plsel.cpp: In member function ‘void nethack_qt_::NetHackQtPlayerSelector::populate_roles()’:
../win/Qt/qt_plsel.cpp:378:53: warning: enumerated and non-enumerated type in conditional expression [-Wextra]
378 | gf = monnum_to_glyph(roles[i].malenum, is_f ? FEMALE : MALE);
../include/display.h:610:27: note: in definition of macro ‘monnum_to_glyph’
610 | ((gnd == MALE) ? GLYPH_MON_MALE_OFF : GLYPH_MON_FEM_OFF))
| ^~~
../win/Qt/qt_plsel.cpp: In member function ‘void nethack_qt_::NetHackQtPlayerSelector::populate_races()’:
../win/Qt/qt_plsel.cpp:407:53: warning: enumerated and non-enumerated type in conditional expression [-Wextra]
407 | gf = monnum_to_glyph(races[j].malenum, is_f ? FEMALE : MALE);
../include/display.h:610:27: note: in definition of macro ‘monnum_to_glyph’
610 | ((gnd == MALE) ? GLYPH_MON_MALE_OFF : GLYPH_MON_FEM_OFF))
| ^~~
