Fix underflow issue on coloring in Stash

[hateborne]

The addresses the issue in #5474.

tl;dr - This moves entirely away from StashGridRange, prioritizing StashList as the sole source of authority.

Stash specific

• All functions in header have doc blocks added, both to aid others in contributing and because I am a psychopath.
• Changed usage of itemId/iv to itemIndex where appropriate. As these were used interchangably, it took a while to untangle things. Anytime the literal index value is used, it should be itemIndex. In other places where the modified id is used, it should be itemId.
• Uses of StashGridRange were replaced with a much smaller loop over only the visible page's StashGrid or the StashList.
• AddItemToStashGrid was removed in favor of an action agnostic function named UpdateStashGrid. This can be used to set an itemId into the corresponding cells, or simply zero out the cells. The important note here is that the item position must be the bottom-left cell. The inconsistency of top-left vs bottom-left is what contributed to some of the issues with stashList getting salty. The PointsInRectangle usage always worked right (x) and down (y), which worked extremely well if the itemPosition was always passed as the bottom-left cell (which is converted within the function before changing things).
• RemoveStashItem now takes an Item pointer, the x/y Point of the item, and the item's index in the StashList. The reason for the passed position versus pulling it from the item is that the autoequip would change the items out before RemoveStashItem was called. This resulted in the itemPosition being incorrect and going out of bounds.
• Removed the multiple manual cursor slot detection loops in favor of the function that already exists to do just that.

Misc

• Uniform invalid item id usage: This is an opinionated change to clarify the code at a glance. While it is functionally the same as uint16_t(-1), it feels like a more obvious explanation of what is happening.
• Simple == and != operators for the Item class.
• Removed the gold withdraw QOL change as it is out of scope for this.

[StephenCWills]

Upon looking at this a little more carefully, the reason for having two loops might have been to avoid drawing the slot back on grid slots after having already drawn the item over those slots. Have you tested this change? Does it look okay?

[hateborne]

(both images)

Good catch! It was tested and it looks ok. I have included an example image using the PR iteration, as well as one with the change you suggested. :)

PR version

With your suggested change

[StephenCWills]

Good catch! It was tested and it looks ok. I have included an example image using the PR iteration, as well as one with the change you suggested. :)

Okay, so the reason it looks mostly correct is because the bottom-left corner of the item sprite is the slot at which the item is placed. It means the slot back is only being drawn over the bottom-right corner of the 2x2 and 2x3 items in your stash. You can only really see it with the studded leather in your screenshot, but it becomes pretty obvious if you highlight an item by hovering with the mouse.

[DakkJaniels]

Shouldn't the two unrelated changes be in separate PR's? (@qndel surprised you didn't comment on that first)

[StephenCWills]

Shouldn't the two unrelated changes be in separate PR's?

It would be nice if they were at least separated into different commits, but they're both sufficiently small changes that I don't think it's that big of a deal. Maybe someone else thinks differently, but I'm willing to let this slide.

[hateborne]

Shouldn't the two unrelated changes be in separate PR's?

It would be nice if they were at least separated into different commits, but they're both sufficiently small changes that I don't think it's that big of a deal. Maybe someone else thinks differently, but I'm willing to let this slide.

I can absolutely do that and apologies for the breach of etiquette/procedure. :)

---

Stephen pointed out the rendering issue on items two cells wide which was the reason for the backs being drawn first. A minimally invasive fix would be to move the code that performs the stashList lookup inside the if statement. It's more relevant for the scope that item is actually used as well.

If you did want to fix the rendering for inventory items so backs can be drawn in one pass that'd be awesome :D, seems like a fair bit of work required though.

I'll review this in the morning and be more diligent in testing before I pester again.

[hateborne]

@StephenCWills I really did not mean for this simple change to end up as a full blown crusade, but here we are. I have tested in every way that caused a crash previously and was unable to cause any issue (read: 0 crashes, 0 item corruption incidents).

In addition to the tests below, the highlighting always worked and the background coloring always worked as expected.

=== Existing Character, Existing Stash ===
Manually placing medium+ size object in corners and cardinal direction 'borders' on blank page
Manually placing medium+ size object in corners and cardinal direction 'borders' on non-blank page
Manually placing medium+ size object over items in corners and cardinal direction 'borders'
CTRL clicking into blank page
CTRL clicking into non-blank page
CTRL clicking into full page
CTRL clicking out of non-blank page
CTRL clicking out of full page
SHIFT clicking out of non-blank page
SHIFT clicking out of full page
Pull gold out
Manually put gold in
CTRL clicking gold in
Saved game, then exited
Loaded game, confirmed changes from previous save & exit

## Deleted Character & Stash saves

=== New Character, New Stash ===
Manually placing medium+ size object in corners and cardinal direction 'borders' on blank page
Manually placing medium+ size object in corners and cardinal direction 'borders' on non-blank page
Manually placing medium+ size object over items in corners and cardinal direction 'borders'
CTRL clicking into blank page
CTRL clicking into non-blank page
CTRL clicking out of non-blank page
SHIFT clicking out of non-blank page
Pull gold out
Manually put gold in
CTRL clicking gold in
Saved game, then exited
Loaded game, confirmed changes from previous save & exit


=== New Character, Existing Stash ===
Confirmed stash exists in state from previous section
Manually placing medium+ size object in corners and cardinal direction 'borders' on blank page
Manually placing medium+ size object in corners and cardinal direction 'borders' on non-blank page
Manually placing medium+ size object over items in corners and cardinal direction 'borders'
CTRL clicking into blank page
CTRL clicking into non-blank page
CTRL clicking out of non-blank page
SHIFT clicking out of non-blank page
Pull gold out
Manually put gold in
CTRL clicking gold in
Saved game, then exited
Loaded game, confirmed changes from previous save & exit


## Restored original Stash save only
(Both new characters could access stash, save & exit, then load & confirm changes)

[ephphatha]

Did you have some more commits you intended to push? You've marked comments as resolved but there's no change to the source visible yet.

[hateborne]

Did you have some more commits you intended to push? You've marked comments as resolved but there's no change to the source visible yet.

Oh nuts, apologies! I forgot to push last two commits. Thank you for your continued patience in this

[AJenbo]

it looks like something has changed game behavior which is why the test are currently failing for this PR.

Could you:

1. merge latest master and see if that helps
2. run the timedemo locally and see if you can see where things diverge (copy the .sv and .dmo from the test fixtures to your save game folder devilutionx --demo 0 --spawn)