Bug - Fixed Issue With restore_mp() Function

[slyz]

Two is to change the code so that it returns the result of the recovery activities, and not continueValue.

I don't think that is necessary. As Jason pointed out, there are cases where not respecting the user's restore settings is allowed.

 

[StDoodle]

One nifty thing I noticed is that the major argument I could see for modifying restore_mp()'s return value would be so that someone else could build a script that directly called restore_mp() after tweaking restore thresholds for niche reasons; however, the way UR stores the "always continue" and "trouble found" settings means they could be tested for in a script without messing things up for someone who ran it without Bale's UR set. Which is nifty to know.

 

[xKiv]

the way UR stores the "always continue" and "trouble found" settings means they could be tested for in a script without messing things up for someone who ran it without Bale's UR set. Which is nifty to know.

That's nifty for people who *know* they are running UR, but not for authors of other scripts, who might just want to call recovery and check the status without having to explicitly consider each known recovery script's return mechanisms (sombody else withes another recovery script few years down the road, suddenly you start getting bug reports ...).
Unless UR is the only recovery script allowed to do that, ever.

 

[StDoodle]

No, it is useful for the authors of other scripts, though the documentation on the wiki should have a note for both functions that mentions the return value can be overriden by a restoreScript, and it's up to a script author to clarify what, if any, such scripts they support. At least UR makes it possible to deal with such situations. One thing that might be nice, though, would be if Bale could post a function that checked for "should have returned false under normal circumstances but didn't because the user explicitly told UR not to" for use by other script authors, but meh.

 

[Theraze]

If you're that paranoid about the values and you're writing a script where you're trying to make sure, you'd just use some code like this:

	if (to_float(my_hp()) / my_maxhp() < to_float(get_property("hpAutoRecovery")))

	if (to_float(my_mp()) / my_maxmp() < to_float(get_property("mpAutoRecovery")))

If it's false, that means that for some reason, your recovery is lower than expected... and if you really really need it to go higher, you override their recovery settings using try/finally so that even if they abort part-way through, you don't permanently screw up their recovery settings.

 

[slyz]

Or simply set baleUr_AlwaysContinue to false.

 

[fronobulax]

I don't think that is necessary. As Jason pointed out, there are cases where not respecting the user's restore settings is allowed.

I disagree but I think this discussion is getting very confused about the existing behavior, the documented behavior, what UR specifically does, what other recovery scripts might do and what might make sense to do once some of the above are clarified and resolved.

Regardless, I do think it is bad form for a function with a specific purpose to return a value based upon the state of the application, rather than the success of that purpose, but I would want to understand continueValue much better than I do before I said that was happening here.

 

[xKiv]

No, it is useful for the authors of other scripts, though the documentation on the wiki should have a note for both functions that mentions the return value can be overriden by a restoreScript, and it's up to a script author to clarify what, if any, such scripts they support. At least UR makes it possible to deal with such situations. One thing that might be nice, though, would be if Bale could post a function that checked for "should have returned false under normal circumstances but didn't because the user explicitly told UR not to" for use by other script authors, but meh.

Bluh. So instead of a useful API that can be used to connect independent pieces of code into greater working whole, we *should* have a mess of 1-to-N contracts? Where you can't plug in part A instead of part B because they have differently wired plugs, even though they do the same thing?

I suspect the correct way to handle this particluar issue is having UR abort(), which mafia should reconize as the wanted third option of "don't try to finish with native restoration, but still return false".

 

[fronobulax]

Dead horse time.

The code is:

	public static Value restore_hp( final Value amount )
	{
		return new Value( RecoveryManager.recoverHP( amount.intValue() ) );
	}

	public static Value restore_mp( final Value amount )
	{
		int desiredMP = amount.intValue();
		RecoveryManager.recoverMP( desiredMP );
		return RuntimeLibrary.continueValue();
	}

Anybody know why hp returns the result from recover but mp returns continueValue? Both flavors of recover are effectively the same - if script and script returns true then true else return success of "internal" restoration.

I played some with blame but a lot of stuff came from 9342 which slyz wrote and I committed and seem to have had some kind of whitespace issue when I did, so it is hard to figure out where the distinction came from. It does, however, seem to be made as far back as r5686.

 

[Veracity]

In my opinion, those should be:

	public static Value restore_hp( final Value amount )
	{
		return DataTypes.makeBooleanValue( RecoveryManager.recoverHP( amount.intValue() ) );
	}

	public static Value restore_mp( final Value amount )
	{
		return DataTypes.makeBooleanValue( RecoveryManager.recoverMP( amount.intValue() ) );
	}

 

[fronobulax]

In my opinion, those should be:

	public static Value restore_hp( final Value amount )
	{
		return DataTypes.makeBooleanValue( RecoveryManager.recoverHP( amount.intValue() ) );
	}

	public static Value restore_mp( final Value amount )
	{
		return DataTypes.makeBooleanValue( RecoveryManager.recoverMP( amount.intValue() ) );
	}

I believe I have updated the wiki to reflect this. I invite more articulate folks to edit my changes but at least the entry that triggered this thread has been tweaked. I also offer up the opinion that this "Bug" be considered closed.