Bug - Fixed Issue With restore_mp() Function

[Roger Ramjet]

According to the function description for restore_mp() at http://wiki.kolmafia.us/index.php?title=Restore_mp

boolean restore_mp( int target )
…
This function will do nothing and return false if you have not checked any restore items. Otherwise, this function returns true if your MP is at or equal to target when finished. If target is set above your current max MP, this function will attempt to restore to that amount and report its success based on that goal, not the number used for target.

The following code, which was part of a larger script, was run.
- Return_Value is a boolean.
- Restore_To is an int indicating the MP level to be restored. It had a value of 980.
- Bale’s Universal_recovery script is used for KoLMafia’s MP restoration.

print(“Using KoLMafia’s restore function.”, “green”);
Return_Value = restore_mp(Restore_To);
print("Return_Value = " + Return_Value + ".");

The reported result was:

Using KoLMafia's restore function.
Restoring MP! Currently at 1071 of 1174 HP, 321 of 1088 MP, current meat: 0 ... Target MP = 980.
Did not fully restore MP for some reason.
Return_Value = true.

At the time the script was run most MP restorative items were selected in KoLMafia, auto-restore was off in KoLMafia, there were no restoratives in inventory, no Meat in inventory, and no adventures available. The restore was expected to fail (the effect of that is what was being tested).

MP was not restored to the requested level (as expected), but restore_mp appears to have returned an incorrect value since MP was not at or above the specified target value when the routine finished. Based on its description, it should have returned “false”.

So, this raises some questions:
1) Did I misunderstand something?
2) Did KoLMafia return the wrong value?
3) Did Universal_recovery have something to do with the apparently incorrect return value?
4) Is the documentation incorrect?

 

[holatuwol]

If you call universal recovery directly, what does it return?

 

[Bale]

The return value of the recovery script conveys information to KoLmafia. If it is false, then KoLmafia will attempt to use its own recovery methods in the eventuality that mp is still under the target. (This includes using items that the recovery script might be intentionally not using.) If the recovery script's return value is true then KoLmafia will not continue recovery regardless of current mp assuming that the script is satisfied with the job done.

For this reason UR will always return true. There are other potential recovery scripts where returning false will do something useful, but UR's mandate will not allow that.

If KoLmafia is simply passing the return value of the recovery script to restore_mp(), then that is probably a bug, or at least the return value does not mean what is posted on the wiki. One of the two.

 

[slyz]

I think Universal Recovery has another way to tell a script it didn't restore fully (BaleUR_trouble or something like this?).

 

[slyz]

I'll mark this as Not a Bug, since everything is working as intended.

 

[Roger Ramjet]

I must disagree most strongly with the assignment of the “Not A Bug” status to this issue. It is an action which is inconsistent with the information in this thread; and the software clearly does not operate as intended.

Based on Bale’s comment above, Universal_Recovery always returns true to prevent KoLMafia from taking further undesired action. Universal_Recovery would seem to be working as intended; but KoLMafia’s handling of this response is consistent neither with the documentation nor the way a user would expect it to work. If the documentation is correct as regards the meanings of KoLMafia’s return value for the function, then there is clearly a problem which should be corrected. If not corrected, it would mean that the return value for restore_mp() (and probably restore_hp()), when a recovery script is used, is either meaningless or wrong, but certainly untrustworthy.

It would appear (based on Bale’s comments) that KoLMafia looks at the return value from a recovery script; if true, it passes it on; if false, it begins its own recovery actions. As Bale states; "UR will always return true." But this does not indicate that the requested recovery has occurred, only that Universal_Recovery does not want KoLMafia to perform any MP restoration. When KoLMafia returns Universal_Recovery’s true response, it is not passing on information consistent with the documented and apparently intended response of restore_mp(), where “true” indicates that the requested MP level has been achieved.

A problem therefore exists in one of two places:

• The interface between KoLMafia and recovery scripts lacks the ability to transmit all of the information required. Instead of a Boolean response, a tri-state value would be needed, where:
  
  1. (current false) means that recovery was not complete and KoLMafia needs to perform its own recovery. restore_mp() response would be based on KoLMafia’s own recovery action.
  2. (current true) means that recovery was complete and KoLMafia does not need to perform its own recovery. restore_mp() response would be true.
  3. (this situation with an inaccurate “true” response) means that recovery was not complete but KoLMafia should not perform its own recovery. restore_mp() response would be false.
  Obviously, this option is not feasible. Not only would KoLMafia have to be changed, but many recovery scripts would also need updating.
  
  
• KoLMafia needs to use the response from a recovery script not as a pass-through value, but as advisory information. If false, execute recovery and return a value based on the results of KoLMafia’s own recovery effort (this is currently done). If true, perform no recovery (this is currently done), but rather (new action), compare the requested recovery level of MP with the actual level of MP and return true or false depending on whether or not the requested recovery level has been achieved.
  
  The second option would seem to be a modest change to KoLMafia which would bring it in line with its apparently intended operation and make the restore_mp() response meaningful in all situations. [The restore_hp() function should also be inspected and modified if it works in the same manner.]

Finally, let us note that this issue is not just about the interaction between Universal_Recovery and KoLMafia. Any recovery script could have the same problem communicating its status to KoLMafia. Thus the fact that Universal_Recovery has (I am assuming this is true without checking) a way to inform the user about its recovery status, is irrelevant, since that reduces the impact of the issue only partially with regard to Universal_Recovery, and not at all with regard any other recovery script or KoLMafia itself.

Therefore as things are obviously not working as intended, I request that the “Not A Bug” status be removed and the problem corrected.

 

[Theraze]

It's not a bug with mafia. If there is a bug, it's a bug with UR, and you should report it there.

If you care for the useful information though, you should just check the values that UR sets based on whether or not recovery was successful.

 

[lostcalpolydude]

I feel like I saw that type of message show up exactly once recently. I don't remember very well, so this is probably a useless anecdote. I don't use a recovery script, I just have mafia set to cast cocoon at some threshold.

 

[fronobulax]

@Roger - I'm going to vent just a little since I'm in the kind of mood where your post smacks of Attitude. The key point seems to be that the code as executed and the documentation disagree. If someone had decided this is Not A Bug then that implicitly says the documentation is incorrect OR there is really nothing wrong. Perhaps Not Going to Fix would have been a status change more to your liking?

However, now that I have released my Inner Snark, it does seem as there is still something wrong. I am having a hard time relating the documentation to the code.

Thinking out loud...

restore_mp calls RecoveryManager.recoverMP, ignores the value that it returns and returns RuntimeLibrary.continueValue instead. Unraveling backwards, continueValue returns values based upon the current status of KoLmafia or the internal interpreter. recoverMP calls a recovery script. If that script fails and returns false then it will attempt to recover using "internal" methods. So the internal recovery will be executed when either a) No recoveryScript is set OR b) A recoveryScript is set and returns false.

Since UR always returns true this suggests that one call to restore_mp will either invoke UR or the internal routines, but not both.

I have not dug deeply into continueValue but my first impressions suggest that it is more concerned with the state of mafia than whether a particular action succeeded or not. So, climbing further out on a limb, my hunch is that it returns true because the script executed or the recovery routines completed without error as opposed to saying anything about whether the task at hand was accomplished.

So, I will paint a target on my back and suggest that a) the documentation is in error (because I cannot find a direct connection between whether the target MP was achieved, or not, and the return value) and b) a feature request to make the recovery routines return true or false based upon whether the MP/HP after restoration is less than the target, or not.

I could also make a case that the reasons for UR to always return true, even when unsuccessful, should be discussed. I do recall some discussion and BaleUR_trouble or something like it was the compromise. My recollection is that always true means that mafia will never try and recover with items that are forbidden to UR, but that's just my memory.

 

[Bale]

It's not a bug with mafia. If there is a bug, it's a bug with UR, and you should report it there.

It is not a bug with UR. If it returned false it might cause an undesirable behavior. The problem is that the return value from restore_hp() and restore_mp() lack the same meaning if a recovery script is being used.

I have not dug deeply into continueValue but my first impressions suggest that it is more concerned with the state of mafia than whether a particular action succeeded or not. So, climbing further out on a limb, my hunch is that it returns true because the script executed or the recovery routines completed without error as opposed to saying anything about whether the task at hand was accomplished.

I believe that is correct. However, a lot of functions that used to return continueValue were changed in the last year to provide a more useful value.

I'll mark this as Not a Bug, since everything is working as intended.

Are you sure that this is what was intended? More importantly, is it still intended since return values for functions like retrieve_item() were changed from continueValue to providing information about actual success.

Perhaps though, since it was intended at the time, what we really need is a feature request.

 

[Bale]

I could also make a case that the reasons for UR to always return true, even when unsuccessful, should be discussed. I do recall some discussion and BaleUR_trouble or something like it was the compromise. My recollection is that always true means that mafia will never try and recover with items that are forbidden to UR, but that's just my memory.

I don't think that really needs to be discussed. I think it currently works properly. If, for instance, someone wanted to make a little recovery script that would decide if it was good to use tasty grubs and beetles to restore in birdform, but if that isn't enough it can return false to pass recovery to mafia's regular healing routines. That saves the script writer from having to account for every healing item in the game by allowing them to make use of mafia's default healing subroutines. This is a good thing.

 

[fronobulax]

I believe that is correct. However, a lot of functions that used to return continueValue were changed in the last year to provide a more useful value.

I was remembering pieces of that. That probably explains the current situation.

I probably should split out what's below and start a feature request discussion, but...

If the internal value is exposed and returned it will be...

If a recoveryScript is present and returns TRUE then TRUE
If a recoveryScript is present and returns FALSE then the results of mafia's internal restore.
If no recoveryScript is present the the results of mafia's internal restore.

While exposing the internal value is "easy" the first case can result in a situation where the MP was not actually restored but restore_mp returns true.

That is acceptable behavior in some cases but it means that folks who use UR will have to check BaleUR_trouble rather than the return from restore_mp. Worthy of more discussion.

 

[Roger Ramjet]

It's not a bug with mafia. If there is a bug, it's a bug with UR, and you should report it there.

If you care for the useful information though, you should just check the values that UR sets based on whether or not recovery was successful.

It is not a bug with UR. UR returns what is required by KoLMafia to get the results the UR needs.

Read Bale’s first post. It should be clear. But, to summerize what Bale said as I understand it;
If UR returns false when MP is not restored as requested, KoLMafia will execute its own recovery actions. UR doesn’t want KoLMafia to do that because UR has already attempted the recovery the way the user requested. Given the boolean response, the only way to stop KoLMafia from executing a recovery is to return true. Therefore the only value UR could ever return is true.

Finally, as I said before, it is not just about UR. The problem arises for any recovery script which doesn’t want KoLMafia to interfere with what it does. So checking any values set by UR only acts as a work-around for scripts using UR. I would like to see the problem solved for all recovery scripts.

 

[fronobulax]

The "bug" is that the wiki is wrong.

Pretend it was changed to read (since I don't have time to edit it at the moment)

restore_mp() will attempt to restore MP according to user preferences concerning how much to restore and what to use.

If a restoreScript is specified and the script runs and returns true then restore_mp will return true. Otherwise restore_mp() will return true if mafia's internal restoration succeeded and false otherwise.

That's what it does. What do folks want it to do?

It's doing the right thing if you don't have a restoreScript specified. What should it do with a restoreScript and why should mafia change, as opposed to the script? In other words is this a problem if the restoreScript returns false when it was unable to restore according to its parameters?

Maybe I've enjoyed too much scotch this evening but this is seeming more and more like a documentation problem. The wiki is wrong. Users of UR do not understand that it is designed to replace mafia's restoration and, as such, users must interact with UR to determine success or failure.

 

[StDoodle]

Does restore_mp() predate restoreScript ability? I seem to recall it does, and that's likely why the documentation is lacking in regards to said scripts; it was probably (judging from my mass-edit memory) a mostly copy-pasta job from info that existed either on the wiki already, or from the first forum thread I was able to find with said info. I know I personally never thought to update it for restoreScript use (mostly 'cause I never really understood how it worked, but also 'cause it never occurred to me to do so).

 

[Bale]

Does restore_mp() predate restoreScript ability?

Yes. When I first started writing a restore script I would test it by using restore_mp() and restore_hp(). Actually I used my rmp and rhp aliases to save my typing fingers, but that is basically the same thing.

 

[jasonharper]

If UR believes that recovery is necessary, and is unable to perform recovery, and doesn't wish to allow built-in recovery to occur... then it should be aborting, rather than returning any value.

A recoveryScript returning true (and therefore preventing built-in recovery), even though the user's recovery target wasn't met, is explicitly allowed; it gives the script the ability to decide that full recovery might not be needed in certain circumstances. Even if you aren't using a recoveryScript, built-in recovery will ignore the target and restore you to 1 HP if you're adventuring in an area with no combats; that is NOT an error, even though it violates the normal contract of the recovery mechanism.

 

[Bale]

Wow. Trust jason to cut straight to the truth of a discussion.

 

[fronobulax]

So there is a two steps to resolve this, right?

One is to edit the wiki so that it says something much like I said in #14 above. Two is to change the code so that it returns the result of the recovery activities, and not continueValue.

Right? If so I can commit the latter for both restore_mp and restore_hp later today.

 

[slyz]

It is not a bug with UR. UR returns what is required by KoLMafia to get the results the UR needs.

UR "fools" Mafia by returning true, and Mafia dutifully passes on that value to the user. It's not KoLMafia's restore_mp() that is fooling you, it is UR.

Bale made this choice for a good reason, and also added a few ways for users to control what happens:

1) Setting the "baleUr_AlwaysContinue" to false (and setting "baleUr_FistPurchase" to true if you are in Fistcore) will make UR abort when restoration failed, instead of returning true.

2) If "baleUr_AlwaysContinue" is true, and restoration fails, UR will return true anyways. This removes the meaning of the return value of restore_mp(), so Bale provided the "baleUr_mptrouble" and "baleUr_hptrouble" properties that scripts could check instead.

This whole issue is with understanding how UR behaves when restoration fails. And yes, the documentation for restore_mp() should be updated with information on recovery scripts.