Angband Forums - Preparing for 4.2 release

Angband Forums (http://angband.oook.cz/forum/index.php)

- Vanilla (http://angband.oook.cz/forum/forumdisplay.php?f=3)

- - Preparing for 4.2 release (http://angband.oook.cz/forum/showthread.php?t=9455)

Quote:

Originally Posted by Nick (Post 139228)

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.

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

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

Quote:

Originally Posted by Sideways (Post 139230)

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 :rolleyes:
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? :D Never mind , looks like the big boss is on it...

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 :

Code:

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 :(

Code:

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:

Code:

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);

}

Quote:

Originally Posted by jml34 (Post 139234)

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 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 :D
I just quickly tested it with

Code:

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 :D

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

Code:

                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 :D

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.

Quote:

Originally Posted by khearn (Post 139240)

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.