Opened 10 years ago

Closed 6 years ago

#1037 closed bug (fixed (in master))

inventory should be sorted before items remaining is displayed

Reported by: anonymous Owned by: molybdenum
Milestone: 3.5.0 Keywords: id
Cc:

Description

When identifying a new item with a scroll, the number of scrolls remaining is listed after it's read, with the current inventory slot. The inventory is then sorted, potentially changing this slot without informing the player of the new slot.

Change History (11)

comment:1 Changed 10 years ago by magnate

  • Milestone changed from Triage to 3.1.3

comment:2 Changed 9 years ago by magnate

  • Milestone changed from 3.2.0 to 3.3.0

Punting to 3.3: non-urgent bug or change.

comment:3 Changed 9 years ago by magnate

  • Status changed from new to confirmed

Confirmed that this still happens as of 3.2rc1.

comment:4 Changed 9 years ago by magnate

  • Milestone changed from 3.3.0 to Future

Punting in accordance with new milestone policy (that any other milestone is only set once someone is actually working on the ticket).

comment:5 Changed 7 years ago by takkaria

  • Milestone changed from Future to 3.5.0

Assigning still-open bugs from Future to 3.5 for fixing.

comment:6 Changed 6 years ago by takkaria

  • Resolution set to fixed (in master)
  • Status changed from confirmed to closed

comment:7 Changed 6 years ago by PowerWyrm

  • Resolution fixed (in master) deleted
  • Status changed from closed to reopened

This change seems to have introduced some new bugs:

  • activatable equipped items instantly recharge
  • reading a scroll of identify from the ground and identifying an equipped item gives a message saying that the identified item is on the ground

This is probably due to a mismatch in do_cmd_use() between the original object and its copy in the code.

comment:8 Changed 6 years ago by molybdenum

  • Owner set to molybdenum
  • Status changed from reopened to assigned

comment:9 Changed 6 years ago by PowerWyrm

While we're at it, maybe the slot info shouldn't be displayed when using up the last item: "You have no more foo." instead of "You have no more foo (slot)."

comment:10 Changed 6 years ago by PowerWyrm

Ok now do_ident_item() reorders the pack before displaying the updated message, but this means that the parameters of the function are no longer valid when the function returns (mainly the current object). This means that any call of do_ident_item() should be isolated and further references to the manipulated object should be avoided.

Looking at the code, do_ident_item() is called five times:

  • in sense_inventory(): called inside a loop, potentially dangerous since a reordering would completely break the loop index; this is really tricky to fix since there is no way to tell how many times the pack will be reordered while sensing it; I don't see any other way than tagging every object in the pack with a flag saying that the object has been processed in the loop and reset the loop each time the pack is reordered; something like
i = 0;
while (i < ALL_INVEN_TOTAL)
{
  object_type *o_ptr = &p_ptr->inventory[i];

  ...

  /* It has already been processed */
  if (o_ptr->tagged) continue;

  if (...)
  {
    do_ident_item(i, o_ptr);
    i = 0;
    continue;
  }

  ...

  o_ptr->tagged = TRUE;
  i++;
}
  • in inven_carry(): two successive calls with the same object parameter, potentially dangerous since the parameter may have changed; easily fixed with an "else"
if (player_has(PF_KNOW_MUSHROOM) && j_ptr->tval == TV_FOOD)
{
  do_ident_item(i, j_ptr);
  msg("Mushrooms for breakfast!");
}
else if (player_has(PF_KNOW_ZAPPER) &&
  (j_ptr->tval == TV_WAND || j_ptr->tval == TV_STAFF))
{
  do_ident_item(i, j_ptr);
}
  • in identify_pack(): called inside a loop, potentially dangerous since a reordering would completely break the loop index; partially fixed already with the "i--" line; replacing that by "i = 0" should ensure that everything is correctly described when identifying everything in the pack (see *enlightenment* effect)
  • in ident_spell(): isolated call, should be fixed by recent commits

comment:11 Changed 6 years ago by takkaria

  • Resolution set to fixed (in master)
  • Status changed from assigned to closed
Note: See TracTickets for help on using tickets.