Preparing for 4.2 release

[takkaria]

Yeah, I think not being able to use an item because something else has its letter in an engraving does seem like it would be a problem at times. There needs to be some way to tell the system to 'ignore engravings for this command'.

Maybe <cmd>'<letter>? So "qq" would quaff the potion engraved with @qq, and "q'q" would quaff the potion in inventory slot q. And of course, if there is no @qq engraving, "qq" would just quaff the potion in slot q.

I think this is over-complicated. If the user has assigned a different key to an item for a particular command, the item that has been overriden should just be assigned another key that isn't in use - I liked the suggestion of uppercase letters.

[khearn]

Yeah, I think not being able to use an item because something else has its letter in an engraving does seem like it would be a problem at times. There needs to be some way to tell the system to 'ignore engravings for this command'.

Maybe <cmd>'<letter>? So "qq" would quaff the potion engraved with @qq, and "q'q" would quaff the potion in inventory slot q. And of course, if there is no @qq engraving, "qq" would just quaff the potion in slot q.

[jml34]

@takkaria If you mean the accesses to s[2] and s[3] they can't be out of bounds (null-terminated string so these expressions are in bound when evaluated) -- I copied this from elsewhere in the code.
Now your code is much cleaner, and faster too. Er, I think you could have a look at some elsewheres in the code
I just quickly tested it with

if (s && sscanf(s, "!L%d", &stack_limit) > 0 && 0<stack_limit &&
                      stack_limit<=gear_obj->kind->base->max_stack) {

and it worked (ignoring !L with numbers>40).
I haven't coded in C for like 20 years so I also tested !L0x20 but it seems %d doesn't accept hexa format

Another quick note (I realize I wasn't clear): I think the player_pickup_aux code would work the same without the 4th parameter and

                if (!pickup_limit) num = get_quantity(NULL, max);
                else num = max;

(don't use auto_max since it's unused). Silly me just got scared of that call to get_quantity that I didn't understand so I left auto_max for no good reason that I know of...

------------
About @<cmd><letter>, my plan would be to have the menu scan the keys, check if they're hidden by an entry in the inscriptions array, in which case the inscriptions array would be scanned for an entry that points to the item: this key would be displayed in the menu if there's one, if not, the first free slot would be assigned first. Degenerate case: the player engraved multiple @ commands to objects so there's no free slot available, we display a space in the menu and the object is not reachable.
@Sideways So @<cmd><letter> could actually be made to work with multiple ways to select an item. Use a key displayed in the menu -> select the corresponding item, use some other key -> select the first engraved item if any.
Now I don't know if that's a desirable feature, the code could be messy, perhaps I can't get back to it before the 1st of 08, and above all other people have said there's a more desirable way to have @<cmd><letter> work so I'll leave it at that for now

[Sideways]

What does that code do? For each hidden item, assign the first free letter?

I think it does that, yes; though there's unrelated but very similar code elsewhere that essentially works by swapping letters between the competing items. (That approach sometimes leads to unintuitive results if an item's letter gets swapped multiple times during the process.)

The fundamental difference between the Frog/Pos code and V is that in Frog every item only has one corresponding key in a given menu (the inscriptions only change what that key is), while V has (so far) tried to make both the original letter and the inscribed key apply within the same menu. I think the latter approach is ultimately unworkable with @[letter][letter] inscriptions allowed, though it didn't cause any conflicts as long as the inscribed keys were always numbers.

(I'd also say removing that is a very cheap price to pay for making @[letter][letter] inscriptions work But that might be because I'm already used to it.)

[takkaria]

if (check_for_inscrip(gear_obj, "!L")) {
    const char *s = quark_str(gear_obj->note); // gear_obj->note is not 0
    while (s) {
        s = strstr(s, "!L");
        /* The stacking limit tag is !L<digit><digit> */
        if (s && '0'<=s[2] && s[2]<='9' && '0'<=s[3] && s[3]<='9') {
            stack_limit = (int)10*(s[2]-'0') + s[3] - '0';
            /* TODO ignore staking limits > 40? */
            break;
        }
        if (s) s++;
    }
    if (stack_limit>=0)
            num_to_pickup = MIN(num_to_pickup, stack_limit - gear_obj->number);
}

This is dangerous code with a potential out of bounds access. I think the following will do what you want just as well:

const char *inscription = quark_str(gear_obj->note);
const char *s = strstr(inscription, "!L");
if (s && sscanf(s, "!L%d", &stack_limit) > 0) {
    num_to_pickup = MIN(num_to_pickup, stack_limit - gear_obj->number);
}

[jml34]

I guess it's OK to double-post for unrelated matters?
I'm playing around with inscriptions for stacking limits and auto_pickup. So {!L25} would only stack up to 25 when walking over a stackable stack of objects.
The get command happily ignores this so far. I think it should prompt for a quantity when picking the most it can would overshoot the limit.

It's not thoroughly tested at all but I thought I'd ask for an opinion. I don't exactly get how the code is organized (lots of similar code in several places) so I'm afraid I'm adding to the mess. It's just a bit annoying when the game keeps stacking things, espacially when it uses a new slot, but no major problem there.

I think it's sane to ignore !Lxx when xx>40. Do people really want to have 60 or 80 of something? It would take more coding to check this out, since as it is, only the first stackable slot is checked out, so if it says !L42, then it will pickup until "it's at 42" which means 2 will go to the next stack each time. I guess I could just check what happens with the next inventory slot...

There's potential for confusion if you manually overshoot the limit, then drop some of them, then change your mind about the limit, then manually pickup items with the previous inscription etc., you end up with several stacks with different inscriptions. Only the first stack is taken into account so if you have 8 items {!L08} then 2 items {!L05}, you won't pickup anything when walking over uninscribed items. Not sure what to make of that. What if there was 7 items !L08, would I have to add 1 item to this stack then again several items to the next? Huh...

---
So the patch I'm trying is in cmd-pickup.c
There's something weird with player_pickup_aux. There are 3 calls to this function. Parameter auto_max is unused 2 times (=0), and is set to inven_carry_num(obj,true) in the 3d, in the do_autopickup function. Now in obj-gear.c we can see that, for the inven_carry_num function, stack=true only serves to return 0 when obj can't be stacked to an existing inventory item.
So either auto_max=0, or the first thing player_pickup_aux does, calling inven_carry_num(obj,false), is to recompute its value.
So auto_max should perhaps be a bool then. But I don't really get in what situation we get to that part of the code with the call to get_quantity so meh.
I need to set a pickup limit somewhere though, so I just added a new parameter :

static void player_pickup_aux(struct player *p, struct object *obj,
                                          int auto_max, int pickup_limit, bool domsg)
{
        int max = inven_carry_num(obj, false);

        /* Confirm at least some of the object can be picked up */
        if (max == 0)
                quit_fmt("Failed pickup of %s", obj->kind->name);

        /* Set ignore status */
        p->upkeep->notice |= PN_IGNORE;

/* new code */
        if(pickup_limit && max>pickup_limit) max = pickup_limit;
/* end of new code */

        /* Carry the object, prompting for number if necessary */
        if (max == obj->number) {
                if (obj->known) {
                        square_excise_object(p->cave, p->py, p->px, obj->known);
                        delist_object(p->cave, obj->known);
                }
                square_excise_object(cave, p->py, p->px, obj);
                delist_object(cave, obj);
                inven_carry(p, obj, true, domsg);
        } else {
                int num;
                bool dummy;
                struct object *picked_up;

/* old code
                if (auto_max)
                        num = auto_max;
                else
                        num = get_quantity(NULL, max); */
/* new code */
                if (!auto_max) num = get_quantity(NULL, max);
                else num = max;
/* end of new code */
                if (!num) return;
                picked_up = floor_object_for_use(obj, num, false, &dummy);
                inven_carry(p, picked_up, true, domsg);
        }
}
[...]
        /* We're given an object - pick it up */
        if (obj) {
                player_pickup_aux(p, obj, 0, 0, domsg); // new code: add a 0 param
[...]
        /* Pick up object, if legal */
        if (current) {
                /* Pick up the object */
                player_pickup_aux(p, current, 0, 0, domsg); // new code: add a 0 parameter
[...]
int do_autopickup(struct player *p)
{
[...]
                        if (auto_pickup_okay(obj)) {
                                /* Pick up the object (as much as possible) with message */
/* Old code 
                                player_pickup_aux(p, obj, inven_carry_num(obj, true), true);
                                objs_picked_up++; End of old code */
/* New code */
                                int num_to_pickup = inven_carry_num(obj, true);
                                /* Check first if there's a stacking limit however */
                                const struct object *gear_obj = find_stack_object_in_inventory(obj);
                                int stack_limit = -1;
                                if (check_for_inscrip(gear_obj, "!L")) {
                                    const char *s = quark_str(gear_obj->note); // gear_obj->note is not 0
                                    while (s) {
                                        s = strstr(s, "!L");
                                        /* The stacking limit tag is !L<digit><digit> */
                                        if (s && '0'<=s[2] && s[2]<='9' && '0'<=s[3] && s[3]<='9') {
                                            stack_limit = (int)10*(s[2]-'0') + s[3] - '0';
                                            /* TODO ignore staking limits > 40? */
                                            break;
                                        }
                                        if (s) s++;
                                    }
                                    if (stack_limit>=0)
                                            num_to_pickup = MIN(num_to_pickup, stack_limit - gear_obj->number);
                                }
                                if (num_to_pickup>0) {
                                    player_pickup_aux(p, obj, num_to_pickup, num_to_pickup, true);
                                    objs_picked_up++;
                                }
/* End of new code */
                        }

(hum, no spoiler tags on this forum? I wanted to hide the code)
On a side note, I probably won't have much time until next month

[jml34]

until code was written to make sure every item always has a letter.

What does that code do? For each hidden item, assign the first free letter?
My quick patch doesn't update the menu keys, which is indeed pretty confusing.
I'll look into that as soon as I'm done with trying to get stacking limits to work
EDIT Ah, so many posts while I was thinking. Er, do I still need to look into it or are knowledgeable people going to do it? Never mind , looks like the big boss is on it...

[Nick]

Thanks all for the feedback, will look into following takkaria's advice.

[Pete Mack]

@tak--
Agree. That stuff belongs under a function pointer, or outside the menu code entirely. It is really mysterious as a menu side effect.

[takkaria]

At first glance this looks good. Anyone see any objections to it (if it works as described)?

It's cool that this is working, but I think a cleaner way to do this would be to set the 'key' member of struct object_menu_data differently when you're building the menu in build_obj_list(). textui_get_item() would have to pass along the 'command key' which can be found from the command via cmd_lookup_key(cmd, mode). This would have the advantage of not touching the menu code at all, and you keep all the logic in the one function. You could keep track of what letters are assigned when you build the list, and mark those that have no letter with '-', or just assign them using uppercase letters starting from A-Z I guess.

[Sideways]

At first glance this looks good. Anyone see any objections to it (if it works as described)?

Poslikes have that; and my experience is that the situation jml34 described (an item being left without a letter slot because its natural letter was claimed by another item's inscription) was actually a fairly serious problem that occurred repeatedly and equally repeatedly produced headaches (especially for new players who hadn't seen it happen before) until code was written to make sure every item always has a letter.

[Nick]

Also thanks for all the helpful suggestions on a new name for Satisfy Hunger. My current thinking is to go with Derakon's idea of Remove Hunger, and change it to not reset food downward. This means it will not be a cure for fullness, which seems more in keeping with the philosophy of the changes. Also makes ,Purging and !Salt Water more useful

[Nick]

I've just tried to patch the game to extend the @[cmd][key] syntax so that "key" can also be a-zA-Z, and it's really great. Try it and you'll wonder how you could play without it!
Engrave default food with @EE, the most useful rod with @aa, wand with @zz, arrow with @tt etc. (roguelike keyset examples) Then other things with whatever is easy to type, and probably use an upper-case key for things you don't want to mistype and launch by mistake.

At first glance this looks good. Anyone see any objections to it (if it works as described)?

[jml34]

I've just tried to patch the game to extend the @[cmd][key] syntax so that "key" can also be a-zA-Z, and it's really great. Try it and you'll wonder how you could play without it!
Engrave default food with @EE, the most useful rod with @aa, wand with @zz, arrow with @tt etc. (roguelike keyset examples) Then other things with whatever is easy to type, and probably use an upper-case key for things you don't want to mistype and launch by mistake.
If I have a potion engraved with @qe then another potion at slot e does get hidden but I don't think it's a problem. I didn't allow for the general @[key] syntax to get extended of course, since this would conflit with the @[cmd][key] syntax.
(The engraving check code doesn't use an efficient algorithm so it's slowed by a factor 6.2 (each @ was checked 10 times, now it's 10+26+26))

ui-object.c

/* old code in 4.1.3
        if (quiver_tags) { */
 /* new code */
        if (quiver_tags && '0'<=tag && tag<='9') {
/* end of new code */
                i = tag - '0';

/* Old code */      
                        ///* Check the normal tags */
                        //if (s[1] == tag) {
/* new code */        
                        /* Check the normal tags when '0'<=tag<='9' */
                        if (s[1] == tag && '0'<=tag && tag<='9') {
/* end of new code */
                                /* Save the actual object */
                                *tagged_obj = obj;

 /* Get inscriptions */
/* old code */
        //m->inscriptions = mem_zalloc(10 * sizeof(char));
        //for (inscrip = 0; inscrip < 10; inscrip++) {
                ///* Look up the tag */
                //if (get_tag(&obj, (char)inscrip + '0', item_cmd,
                                        //item_mode & QUIVER_TAGS)) {
/* new code */
        /* inscriptions: '0'-'9', 'a'-'z', 'A'-'Z' */
        m->inscriptions = mem_zalloc((10+2*26) * sizeof(char));
        for (inscrip = 0; inscrip < 10+2*26; inscrip++) {
                /* Look up the tag */
                char tag;
                if (inscrip < 10) tag = (char)inscrip + '0';
                else if (inscrip < 10+26) tag = (char)(inscrip-10) + 'a';
                else tag = (char)(inscrip-10-26) + 'A';

                if (get_tag(&obj, tag, item_cmd, item_mode & QUIVER_TAGS)) {
/* end of new code */
                        int i;
                        for (i = 0; i < num_obj; i++)
                                if (items[i].object == obj)
                                                break;

file ui-menu.c:

/* Old code: we don't touch the first if but we need to swap the ifs        
        if (menu->flags & MN_CASELESS_TAGS)
                key.code = toupper((unsigned char) key.code);

        if ((menu->flags & MN_INSCRIP_TAGS) && isdigit((unsigned char)key.code)
                && menu->inscriptions[D2I(key.code)])
                key.code = menu->inscriptions[D2I(key.code)];
*/ /* new code */
        if (menu->flags & MN_INSCRIP_TAGS) {
                unsigned char keyc = (unsigned char)key.code;
                int index = -1;
                if ('0'<=keyc && keyc<='9') index = keyc-'0';
                if ('a'<=keyc && keyc<='z') index = (keyc-'a')+10;
                if ('A'<=keyc && keyc<='Z') index = (keyc-'A')+10+26;
                if (index>=0 && menu->inscriptions[index])
                        key.code = menu->inscriptions[index];
        }

        if (menu->flags & MN_CASELESS_TAGS)
                key.code = toupper((unsigned char) key.code);
/* end of new code */

I hope it's OK to not learn git for a simple patch like this sorry
Well above all I hope it doesn't break things. I made a beginner warrior and whatever I tought I'd try worked...

[Chud]

Normalize Nutrition

Moderate Gluttony

Frodo's Second Breakfast