showed non-empty containers in inventory (including the one being
applied) with a 'for sale' suffix during put-in operations, as if the
shop was trying to sell it to the hero. Amount shown was cumulative
value of its contents. (Using /menustyle:T doesn't show the container
being applied so this wasn't visible with it unless other non-empty
containers were being carried.)
Two or three fix attempts solved one problem but introduced another.
This one seems to finally get things right but considering that there
was trial and error along the way, my confidence isn't great.
Do late message suppression in a different fashion. Also, there are
more messages than shk taking hero's possessions and guard taking
hero's gold that need to be suppressed if regular message delivery
is no longer possible: "do not pass Go", "you arise from the grave
as a foo", "the corridor disappears", "you are encased in the rock".
Those last two are from vault handling but take place in a convoluted
manner: paygd -> mongone -> grddead -> clear_fcorr.
Closing nethack's window sets 'program_state.stopprint' to inhibit
disclosure interaction, but shopkeeper claiming hero's stuff or vault
guard claiming hero's gold didn't honor that and just issued normal
pline messages. For win32, they got delivered in a popup even though
nethack's window had gone away.
Make those two end-of-game situations honor 'program_state.stopprint'.
[Fix not tested on win32...]
Lock context wasn't being cleared if it was for a container and that
container got destroyed. Case discovered was forcelock() ->
breakchestlock() -> delobj() (sometimes the container is destroyed
rather than just breaking its lock) followed by #wizmakemap (replace
current level) and maybe_reset_pick() trying to check whether
xlock.box was being carried. But being interrupted, destroying the
container or dropping it down a hole to ship it to another level, then
attempting to resume picking the lock would also find a stale pointer.
Items on floor in the free spot one step inside a shop's doorway were
showing shop sell prices. Treat items on that spot as if they were
flagged no_charge as on the floor of other shop squares.
Report stated that sometimes they showed a 'for sale' price and
sometimes they didn't, but I didn't see any cases where they didn't.
More shop price determination fallout. After the most recent change
to get_cost_of_shop_item(), using ':' inside an engulfer carrying at
least one item while inside a shop would try to follow the item's
obj->ocontainer back-link and crash when that led to the engulfing
monster rather than to a container.
The recent attempt to have looking inside a container show shop
prices had multiple problems. Worst one was showing shop prices as
if the hero would be buying for items already owned by the hero.
Item handling inside containers on shop floor was inconsistent: if
shop was selling those items, they would include a price, but if not
selling--either already owned by hero or shopkeeper didn't care about
them--they were only marked "no charge" if hero owned the container.
This is definitely better but I won't be surprised if other obscure
issues crop up. Gold inside containers on shop floor is always owned
by the shop (credit is issued if it was owned by the hero) but is not
described as such.
When merging one stack into another and they have different obj->o_id
price adjustments, keep the o_id of whichever one commands the higher
shop price.
I misread part of the original code and the revision introduced a bug
based on that. obj->o_id price variations are used for all types of
non-IDed items, not just non-glass gems.
Player came across a stack of 2 gray stones in a shop and kicked one.
That one ended up with a different (in his case, lower) price once it
was separate. This behavior only applies to non-glass gems which add
a price variation derived from internal ID (obj->o_id) number. Make
splitting stacks always yield the same price per item in the new stack
as was being charged in the old stack by choosing a similar o_id. Do
it for all splits (that can vary price by ID, so just non-glass gems),
not just ones performed inside shops.
He picked up the lower priced one and dropped it back on the original
higher priced one; the combined stack took on the lower price. That
will no longer happen if they come from splitting a stack, but this
fix doesn't address merging with different prices when they start out
as separate stacks. (Unpaid items won't merge in inventory if prices
are different, but shop-owned items will merge on floor.)
Fix another inconsistency with containers in shops: prices shown when
looking inside. Apply had them (because shop goods in containers are
flagged as 'unpaid' when hero carries the container), and loot did not
(because they aren't flagged that way).
stolen_value() treated hero-owned container holding shop-owned goods
as free for the taking.
The fix I'm working on which led to discovering this first added
stolen_value() then eventually stopped using it so I don't have an
example of where it is giving the wrong result.
get_cost_of_item() was giving different information from shop #chat
when dealing with containers owned by hero containing objects owned
by the shop. And when it was legitimately reporting a price of 0,
doname_with_price() wasn't reporting 'no charge' for items inside a
shop that were owned by hero or that shopkeeper didn't care about.
Extend the shop price reveal to far-look, but only when hero and item
being examined are inside the same shop.
Another one from 6.5 years ago, identifying a type of gem should give
a new price for any unpaid gems of that type and adjust shopping bill
accordingly. Report was for rubbing with touchstone and learning
worthless glass with price not changing until the learned 'gem' was
dropped. Fix works for that and also other forms of identification
(and for amnesia, raising prices of forgotten gems); no dropping is
required for the price to change.
Theoretically could apply to any type of item, but prices of gems are
by far the most sensitive to whether or not they're identified.
I thought that the earlier fix for #H2504 was too easy for anything
shop related. It didn't deal sensibly with containers owned by hero
but holding unpaid shop goods.
Prevent food detection--scroll or crystal ball--from noticing the cat
corpse inside SchroedingersBox since its presence is tentative and
resolving its status during detection is a huge can of worms (live cat
placement on map from inside locked box, parallel resolution required
for monster detection/warning/telepathy that would render the box
fairly useless since it would probably end up getting resolved by ESP
before hero gains access).
Prevent cat corpse in the Box from being added to shop bill if unpaid
Box is picked up. That prevents it from being listed as a bought item
if the player buys the box (instead of being described as unknown
contents; an older, more general bug which still hasn't been fixed).
As far as I'm aware, off the revised handling of Schroedingers Cat is
finished.
When paying for shop door or wall damage, if the entire amount was
covered by shop credit then impossible "zero payment in money2mon"
would occur as the shop code tried to transfer 0 zorkmids from hero
to shopkeeper after using credit to pay.
Most shop messages accurately identify the shopkeeper even when he
or she can't be seen, but some also include a pronoun reference that
ended up as "it" or "its" when not seen. Extend pronoun selection
so that visibility can be ignored: noit_mhe(mon), noit_mhim(mon),
and noit_mhis(mon). Note that despite being called noit_foo(),
those will still return "it" if mon is neuter.
"Accurately identify shopkeeper" is misleading if the hero is
hallucinating; a random shopkeeper name is used then. noit_foo()
yields the pronoun applicable to the actual shopkeeper and might
not match the gender of a hallucinatory name. That could be fixed
in a couple of ways (add shk_mhe()/shk_mhim()/shk_mhis() and either
pass them the randomly chosen name so that they can figure out the
appropriate gender, or just have them use a random gender whenever
hallucinating) but I don't think that's worth bothering with.
A bunch of shop messages needed noit_foo(); only a couple of those
have actually been tested. A bunch more were using shkname() at
the beginning of a sentence where Shknam() should be used instead.
(All the existing shk names are already capitalized so there's no
noticeable difference.)
The three places outside shk.c and vault.c which directly use
pronoun_gender() have been successfully tested.
Fixes#121
Fix githib issue #121 - shopkeepers don't charge for consecutive
acts of vandalism on the same square. pay_for_damage() keys its
action on the 'when' field of the damage structure, and when a
second type of damage gets added to existing damage, that wasn't
being updated. Both bits of damage (broken door or dug wall plus
trap created at same spot) get repaired together but shopkeeper
wasn't challanging hero to pay for the second one (trap).
The repair process had issues too. If you broke a shop door, paid
off the shopkeeper, then left the level before the repair took
place and came back after (or rather, catching up for lost time
repaired it when you returned to the level), it didn't actually
get fixed and remained a doorless doorway that was considered to
have been successfully repaired (record of damage discarded).
Unless there was also a trap there; then the door did get properly
fixed when the trap was removed.
Object scattering during wall repair was bypassed if trap removal
took place.
Also, trap removal while off level still gave messages when it took
place after you returned. I didn't try to verify that; it's possible
that vision is in a state where you can't see the repair even if you
return to a spot where it would be visible. But the return value
from the repair routine was one which wanted a message instead of
the one to suppress messages.
Not addressed: digging a pit inside a shop (aside from in doorway
or breached wall) is not treated as damage which should be repaired.
This includes the case of digging up a grave which converts the spot
into ordinary floor (plus pit trap).
Stealing a shop object from outside the shop with a grappling hook
would result in that item being left marked 'unpaid' after the shop's
bill was treated as being bought and not yet paid for. This led to
"unpaid_cost: object wasn't on any bill" every time inventory was
examined. The problem was caused by handling the shop robbery after
removing the object from the floor but before adding it to inventory,
so it couldn't be found to have its unpaid bit cleared.
When investigating this I came across a more severe bug: if the hero
had never entered the shop, the shopkeeper's bill wasn't initialized
properly and add_one_tobill() could crash while attempting to execute
bp->bo_id = obj->o_id;
because 'bp' was Null.
The #tip command tries to reduce verbosity by formatting drop messages
with just the object name instead of with full sentences, yielding
Objects spill out: obj1, obj2, obj3, ..., objN.
where the trailing comma or period is included with each successive
object. If an intervening message occurs, such "25 zorkmids are
added to your credit", the rest of the objects will no longer be
extending the original sentence and end up looking silly.
Objects spill out: obj1, obj2,--More--
25 zorkmids are added to your credit. obj3, ..., objN.
This fix causes the post-interruption messages to revert to verbose
format.
Objects spill out: obj1, obj2,--More--
25 zorkmids are added to your credit.--More--
obj3 drops to the floor.--More--
...
objN drops to the floor.
The interrupting message still follows the comma of the partial
sentence, but I don't see any sane way to fix that other than to
abandon the terse format altogether, and doing that makes #tip way
too verbose when the container has a lot of items in it. But #tip
inside shops now does that, since there will always be buy/not-
interested feedback interrupting the terse format in that situation.
For other situations, a full sentence message might end up following
a partial sentence list of dropped items.
There was a more significant bug. Dropping a hero-owned container
with gold in it onto shop floor sold the gold to shk, giving hero
credit. Subsequent #tip gave the hero credit for that same gold
when it spilled out. addtobill(obj) relies on obj->ox,oy to
determine whether events are taking place in a shop, and #tip was
relying on placement onto floor to set those, too late for shop
billing. The fix yields suboptimal results: you're given credit
when you drop the container, then during #tip when you spill the
contents, credit for the gold is removed, then new credit for it
is given. That's down to shop insanity, not tipping behavior.
Zapping wand of undead turning at self while inside a shop and
carrying a corpse caused the shopkeeper to claim a use-up fee for
the corpse regardless of whether it was owned by the shop.
Not mentioned in the report: casting stone-to-flesh as self while
carrying a figurine or statue behaved similarly.
To test:
1. Get a level layout with two shops facing each other, e.g. minetn-4.
2. Sell a fragile object to one of the shops.
3. Dig a pit in the other shop's door space so its shopkeeper stays out
of the way.
4. Pick up an object in that other shop so it appears on your bill.
5. Zap a wand of striking at the first shop to break the fragile
object.
6. 'p'ay for the object picked up.
Expected result: Object gets the standard prompt to pay for it.
Actual result: "Paid object on bill??" followed by "Program in disorder
perhaps you'd better #quit." followed by the object being given to the
player for free.
The cause? This comment going all the way back to 2002:
> /* FIXME: object handling should be limited to
> items which are on this particular shk's bill */
Originally reported by PaRaD0xx in FreeNode's #NetHack IRC channel
whilst playing NAO343.
Based on DynaHack commit d995ed1 (Fix paid object on bill when angering
another shkp) by me.
It's obviously supposed to be the latter and not the former.
Interesting note: This same bug was found and fixed in NitroHack commit
4973ce4 (static checker day: fixes for scan-build and PVS warnings).
There's no capacity for the shop logic to handle gold without also
changing the credit/debit within it, so gold must always be handled in
`sellobj()`, even when the state of it is set to `SELL_DONTSELL`.
This is needed for an upcoming bug fix.
Based on DynaHack commit b0784c5 (Credit/debit gold in containers even
in sellobj_state SELL_DONTSELL) by me.
For a shop to NOT charge for an object, two conditions apply:
1. The object's `no_charge` flag must be set.
2. That `no_charge` flag must be set regardless of whether or not the
shop typically sells the object in question.
There are two places in `sellobj()` which ignore the second condition,
thus transferring object ownership from the player to the shop without
the player's consent:
1. A container is dropped in a shop that typically sells such
containers and `sellobj_state` is `SELL_DONTSELL`.
2. A zero-cost container holding nothing but gold is dropped in a shop
that typically sells such containers.
Neither occurs currently in NetHack: the latter because NetHack has no
zero-cost containers, but the former is needed for an upcoming bug fix.
This may be related to SC343-21: "Accounting is incorrect for containers
dropped in a shop that does not sell them."
Based on DynaHack commit 4e79b6a (Don't shop-donate non-empty bags
dropped in sellobj_state SELL_DONTSELL) by me.
Most shop messages use shkname() to give the shopkeeper's accurate
name (or hallucinatory substitute) even if he or she can't be seen.
stolen_value() was using mon_nam(), which calls shkname() if the
monster is a shopkeeper who can be seen, but produces "it" when not
seen. Change it to use shkname() like the rest of the shop routines.
Also, replace Monnam() (quite a few instances) with new Shknam() to
do the same duty when the name is at the start of a sentence.
There was also a very obscure bug where if you could see two
shopkeepers at the same time, you could probe the map one spot at
a time with repeated use of the 'p' command to locate monsters in
general and other shopkeepers in particular. Very tedious and not
very useful, but now fixed.
Putting gold into a hero-owned container on a shop's floot gave credit
for the amount of the gold but also set the gold object no_charge, so
it could be taken out without taking away the credit. Then put back
in and taken out as many times as the player liked, doubling the gold
each time until the shopkeeper was out of cash.
I think the proper fix would be to avoid giving credit instead of not
marking the gold no_charge, but that would require multiple additional
changes so I took the easy way out.
Most of the changes to pickup.c are reformatting that it escaped prior
to release. The changes to shk.c are cosmetic and not part of the fix.
Explicitly combine adjacent string literals so that pre-ANSI compilers
still have a chance to compile the code. I thought these had already
been dealt with, but I kept stumbling across them while reformatting,
so am trying to get them all out of the way now.
Last few && or || followed by end-of-line comments, plus tab replacement
and 'return' parentheses. Not as many of those; some of these files had
already had that done.
Also, tweaked non-cursed scroll of charging read while confused to be a
tiny bit more effective.
To do: find and fix block comments that immediately follow a line with
an end-of-line comment and got misindented to line up with that comment.
I couldn't figure out why walking over a container in a shop might
give the wrong price; the code looks correct. But I've reorganized
get_cost_from_price to perform the cheapest tests first. The u.ushops
check should probably be done in doname to avoid calling this routine
at all 99.99% of the time.
