Assert in object_absorb_partial()

[PowerWyrm]

In my variant, I go to the general store and buy stacks of 40 arrows until quiver is full then one more stack. At this point, object_absorb_partial() triggers an "assert(newsz2 < obj1->kind->base->max_stack);"

When I try the same in V, I don't get the assert (I suppose there is no combine pack in this case, so the bug is hidden). Looking at the code, when combine_pack() is called, I don't understand why it tries to combine full stacks at all, this makes no sense. Until the quiver is full, nothing happens (because there is no assert in this part of the code), although the code fuses the properties of two stacks of arrows for no reason, calling object_absorb_merge() on the two stacks that have no reason to be merged. Of course the assert triggers when the quiver is full since it now tries to merge one stack in the quiver and one stack in the pack that are both of size 40.

[PowerWyrm]

I added the following check at the end of object_stackable() and it seems to fix the problem:

if (obj1->number == obj1->kind->base->max_stack) return false;
if (obj2->number == obj2->kind->base->max_stack) return false;

[Nick]

Thanks, I was already thinking there was some unnecessary combining going on.

[backwardsEric]

I added the following check at the end of object_stackable() and it seems to fix the problem:

if (obj1->number == obj1->kind->base->max_stack) return false;
if (obj2->number == obj2->kind->base->max_stack) return false;

Some of the uses for object_stackable() in Vanilla are using it for the similarity tests and assuming it's not testing the numbers involved (in cmd-pickup.c for instance). Should there be a separate function for that (there's already object_similar(), but it calls object_stackable() and also has it's own test on the numbers)? An alternative would be to not modify object_stackable() but change the tests in obj-gear.c's inven_can_stack_partial() so they reject trying to combine two stacks already at their limits (it looks like changing the tests for "remainder > limit" to "remainder >= limit" and "remainder > obj->kind->base->max_stack" to "remainder >= obj->kind->base->max_stack" would be sufficient).

[PowerWyrm]

Some of the uses for object_stackable() in Vanilla are using it for the similarity tests and assuming it's not testing the numbers involved (in cmd-pickup.c for instance). Should there be a separate function for that (there's already object_similar(), but it calls object_stackable() and also has it's own test on the numbers)? An alternative would be to not modify object_stackable() but change the tests in obj-gear.c's inven_can_stack_partial() so they reject trying to combine two stacks already at their limits (it looks like changing the tests for "remainder > limit" to "remainder >= limit" and "remainder > obj->kind->base->max_stack" to "remainder >= obj->kind->base->max_stack" would be sufficient).

Yeah I saw those occurences, but I don't think it matters, since it's to test if an object can be picked up using match in inventory... but if you try to pick up an object with max stack number or if you have an object with max stack number in inventory you won't ever be able to match them.

[backwardsEric]

In Vanilla, the assert can be triggered when the quiver is full and a partial stack is bought that's similar to a full stack in the quiver. There's a proposed fix up as a pull request.

Adding some test cases to exercise combine_pack() would be useful, regardless of how this specific problem is fixed.