Bug - Fixed currentSpleenUse after failing to use multiple spleen items

[roippi]

Starting with 4 actual currentSpleenUse, and manually setting currentSpleenUse to 0 (i.e. simulating using a spleen item outside of mafia):

> ash available_amount($item["Agua de Vida"])

Returned: 26

> set currentSpleenUse=0

currentSpleenUse => 0

> use 3 agua de vida

Using 3 agua de vida...
Your spleen might go kablooie.

> ash available_amount($item["Agua de Vida"])

Returned: 26

> get currentSpleenUse

12

Likewise if my actual currentSpleenUse is 8, if I set currentSpleenUse to 0 and try to use 2 items it will fail and set my spleen to 8. You can imagine other undesirable combinations; erroneously attempting to use 20 wads on an empty unsteeled spleen will set it to full.

Perhaps if a multi-use fails, UseItemRequest could attempt to single-use?

 

[slyz]

Here are lines 1804-1805 of UseItemRequest.hava, where currentSpleenUse is updated to take into account the failure to consume:

int spleenHit = ItemDatabase.getSpleenHit( item.getName() ) * item.getCount();
int estimatedSpleen = KoLCharacter.getSpleenLimit() - spleenHit + 1;

if ( estimatedSpleen > KoLCharacter.getSpleenUse() )
{
	Preferences.setInteger( "currentSpleenUse", estimatedSpleen );
}

In you first example, spleenHit should be 12, and estimatedSpleen should then be 4. In you second example, spleenHit should be 8, and estimatedSpleen should then be 8.

The problem seems to be the "if ( estimatedSpleen > KoLCharacter.getSpleenUse() )". getSpleenUse() takes into account the items you have just tried to consume, so in your first example getSpleenUse() is 12 and in the second example, 8.

I can't really see why we should check for estimatedSpleen > KoLCharacter.getSpleenUse(). Thoughts?

 

[Veracity]

We had a similar issue with rolling back fullness when you try to eat too much.
Unlike spleen items, KoL will consume some of the food and balk before eating the extras.
With spleen, it will use none of them

Look farther down the page at the "too full" handling in lines 1844-1888 of UseItemRequest.java.
In particular, look at lines 1858-1860:

			// Roll back what we did to fullness in registerRequest
			int count = item.getCount();
			Preferences.increment( "currentFullness", -fullness * count );

where it rolls back the fullness before lines 1877-1882:

			int estimatedFullness = maxFullness - fullness + 1;

			if ( estimatedFullness > KoLCharacter.getFullness() )
			{
				Preferences.setInteger( "currentFullness", estimatedFullness );
			}

...which is similar to the the spleen lines you cited.

UseItemRequest.registerRequest() on lines 4550-4554 DOES increment the spleen.

Therefore, you need to take that into account when rolling it back, just as we do with food items.

(This is a perfect example of why I think the "correct" way to keep the object model in synch is to not change it when we submit the request - registerRequest - but when we have the actual response and can see what happened - parseResults, or the equivalent, for each kind of request. All requests started out the first way, but, after fixing more and more and more cases of improper rolling back upon failure, I ended up refactoring most requests to use the "correct" method.

UseItemRequest is one of the ones I did NOT refactor. It is complicated. I encourage you to submit the spleen rollback fix - and then, at your leisure, see if you can refactor it to NOT modify the object model in registerRequest and instead do all the updating in parseConsumption.

Ironically, I also never refactored CoinMasterRequest to have the "correct" technique: I deduct tokens or sold items when we make the request and, if it fails, "refund" them. So, even thought that code has been heavily whacked recently, it still doesn't use my preferred technique. :-/)

 

[roippi]

Hah. I was just about to paste that " // Roll back what we did to fullness in registerRequest" line. That one comment helped me a lot when I was implementing distention pill stuff (which incidentally, I think is the "correct" way if I'm grokking what you said. We submit the request as normal and if we parse out a distention message, we decrement fullness by 1).

I think, perhaps, we can be somewhat intelligent about what we roll spleen back to (or even forwards to) in parseConsumption. Example:

currentSpleenUse is 0 and we submit 6 spleen for consumption and get a "rupture" message. Current behavior leaves currentSpleenUse at 6; just rolling it back would set currentSpleenUse back to 0. Instead, we know that there is a maximum of 5 room left in your spleen, so set it to 10. (i.e. KoLCharacter.getSpleenLimit() - spleenHit + 1)

edit: wait. Now that I look at the code that slyz posted, that seems to be exactly what that code was trying to do. The problem seems to be exactly the line that slyz pointed out: if( estimatedSpleen > KoLCharacter.getSpleenUse() ). I agree, I can't see a use for that conditional.

 

[slyz]

I encourage you to submit the spleen rollback fix

r9524 (untested)

then, at your leisure, see if you can refactor it to NOT modify the object model in registerRequest and instead do all the updating in parseConsumption.

I'll try to do that, by and by. I still don't have much time to devote to KoL or Mafia these days =/

 

[Veracity]

Can we mark this fixed?

 

[Veracity]

Yes.

> get currentSpleenUse

8

> set currentSpleenUse=0

currentSpleenUse => 0

> use 2 coffee pixie stick

Using 2 coffee pixie stick...
Your spleen might go kablooie.

> get currentSpleenUse

8