Feature - Implemented Include variable/scaling monsters in monster stat functions

[nudely_appendage]

Oh, I thought Veracity's comment was on the lines of:

If you are using single characters to represent specific values,
Then you need your own "namespace" to avoid confusion.

If you don't use single characters, then there is less likely to be confusion or clashes. I haven't used eval; is there any chance that someone might need player stats in other contexts?

 

[Rinn]

If combat item damage was added as a data file it would need stat values as well, also a bunch of other variables Zarqon needed for his expressions. Otherwise no, there currently isn't a lot of overlap, but that doesn't mean there couldn't be in the future.

 

[Theraze]

With the way I have it working currently, adding a new evaluative section is as difficult as defining a new type at the top of the Evaluator file, putting in your new A-Z (uppercase) sections, and pointing to your new type in your file... Should take under 5 minutes for most additions.

 

[jasonharper]

Theraze, nobody can evaluate your evaluator, because it isn't actually included in the patch - only your changes to existing files are there, you have to "svn add" new files for them to be considered for diffs.

It doesn't look very promising, based on the fact that you've got a single constructor taking a flag for which expression dialect to use, rather than subclasses for each dialect. I can only assume that you're checking that flag all over the place - the modifier expression evaluator is a fairly speed-critical part of the program (especially when the Maximizer is being used), please don't do anything that would significantly slow it down.

 

[Theraze]

Well, that explains the patch size. Here's one that has it. Showed unversioned files when making the patch and it gave me the right file.

Didn't notice any speed issues when I was running, but I'm open to modifying it however.

 

[Veracity]

...you've got a single constructor taking a flag for which expression dialect to use, rather than subclasses for each dialect. I can only assume that you're checking that flag all over the place...

Right. Look, maybe this is OOP 101, but what I'd like to see is something like:

class Evaluator

This understands arithmetic. It has a set of built-in arithmetic functions. Perhaps it has some built-in variables - single letters. Perhaps not. It provides two methods (which subclasses can override): evaluateFunction and evaluateVariable. It calls these when it finds a function name or variable which is not built-in to this class. The default behavior is to report a syntax error.

class ModifierEvaluator extends Evaluator

evaluateFunction handles Modifier specific functions. evaluateVariable handles Modifier specific variables.

class MonsterEvaluator extends Evaluator

evaluateFunction handles Monster specific functions. evaluateVariable handles Monster specific variables.

So, the first task in refactoring to this structure is to take the Modifier Evaluator and break it into two parts: the general arithmetic evaluator, and the subclass with the modifier-specific functions and variables.

Having done that, making a Monster Evaluator and using it to parse expressions in monsters.txt should be trivial.

Note that this will mean that you will have three new files: Evaluator.java, ModifierEvaluator.java, and MonsterEvaluator.java. You will not have a flag in your constructor telling you which one to make. You will, therefore, not check a flag all over the place and do different things depending on what subclass you are.

 

[Theraze]

So yeah... my current issue is trying to completely rework the eval() function. I'm confused by the expect/until/optional/optional/expr/term/factor/value part. I don't see where they're getting used by the top in Expression or eval, though I know they have to be there somehow... I see that Expression has this.bytecode which calls expr(). Is that the only one that calls those private functions? Am I missing it? Sorry... I've put together the 3 files, the other 2 classes are extending the original Evaluator class. modifier_eval shows me that the ModifierEvaluator class is working as it should. The original code of the evaluate is what still needs to be redone.

 

[Rinn]

I made Expression able to handle multiple copies of the same text function. I also added basestat and buffstat as a proof of concept. I'm also pretty sure I fixed a bug with the resizing of the stack array, it seems it was only copying the first 10 elements when resizing it, which would have worked until it needed to be resized a second time. Spun a new patch.

> ash my_basestat($stat[moxie])

Returned: 2095

> ash my_basestat($stat[muscle])

Returned: 1518

> ash modifier_eval("basestat(mox)")

Returned: 2095.0

> ash modifier_eval("basestat(mus)")

Returned: 1518.0

> ash modifier_eval("basestat(mox)+basestat(mus)")

Returned: 3613.0

 

[Theraze]

Okay, added the multiple expression part to the Evaluator class and its two (current) spinoffs. Still not sure how I'm going to rework those functions to recurse properly. Would help if I had a background in Java, and wasn't selftaught in the last... how long have I been making patches for mafia? Ah well, so it goes. May be a while before I get the original function recoded. We'll see.

 

[Theraze]

Decided to update my personal monster files to test this. Discovered that, as somewhat expected, mafia loads the monster database on program launch, not on character load, which makes sense... but means that any scaling mob generally had stats of 0. Made it save the (up to 4) strings, if they were defined, to the monster record. If length of a string is greater or equal to 2, on checking monster.getAttack or any of those bits, it reruns the evaluator string. If the bool check bails, it returns the static value. Haven't noticed speed issues with it yet tonight.

 

[Darzil]

Added Hipster encounter scaling logic to my first page post on scaling monster stats.

 

[Rinn]

If length of a string is greater or equal to 2, on checking monster.getAttack or any of those bits, it reruns the evaluator string.

You should just be checking if the string is not an integer.

 

[Veracity]

You should just be checking if the string is not an integer.

I would suggest that you use the same convention as in modifiers.txt: the value of a field (hp, atk, def, init, whatever) is either an integer, or is an expression enclosed in square brackets.

If it's an integer, parseInteger on it, set the constant value, and the expression string is null.
If it's an expression, save the expression string - or, perhaps, the byte-compiled expression.

When a function is called to get a monster's attack, defense, hp, whatever, if the expression string is null, return the constant value. Otherwise, evaluate the expression.

In other words, don't bother saving expression strings unless it's really an expression. If it's not an expression, evaluate the constant exactly once.

 

[Theraze]

I would suggest that you use the same convention as in modifiers.txt: the value of a field (hp, atk, def, init, whatever) is either an integer, or is an expression enclosed in square brackets.

Running the token (the string) through isNumeric. If it passes, it saves the value to int using parseInteger. isNumeric is what parseInteger uses to decide whether or not it's valid as well, so that should be a safe test. If it doesn't pass, it saves the token (string) to an evaluate<type> string. So it saves int health if it passes, String evaluatehealth if it fails.

When a function is called to get a monster's attack, defense, hp, whatever, if the expression string is null, return the constant value. Otherwise, evaluate the expression.

In other words, don't bother saving expression strings unless it's really an expression. If it's not an expression, evaluate the constant exactly once.

Exactly what I have it doing currently. Though with what Rinn said, I was considering just saving the string in all cases and evaluating its merits on each call...

The only question I currently have is regarding whether there's a better way to check if the expression string is null rather then checking for length. Can I just do "if (this.evaluatehealth != null)" or something like that? I know with C languages, that'd cause all sorts of chaos... that's why I went the string length route, as I'd seen it used for checking null string in current parts of the existing Evaluator function.

 

[Veracity]

Exactly what I have it doing currently. Though with what Rinn said, I was considering just saving the string in all cases and evaluating its merits on each call...

This is exactly what you are doing? Saving the string, if the expression is in square brackets, or evaluating it as a number and leaving the string null otherwise? Good, if so, although your immediately following paragraph makes me wonder...

The only question I currently have is regarding whether there's a better way to check if the expression string is null rather then checking for length. Can I just do "if (this.evaluatehealth != null)" or something like that? I know with C languages, that'd cause all sorts of chaos... that's why I went the string length route, as I'd seen it used for checking null string in current parts of the existing Evaluator function.

If a string is null - a null pointer - you CANNOT check for length. I don't understand the "cause all sorts of chaos" comment. That's a perfectly normal way to check if a pointer is null in all C-like languages. Whether or not a pointer is ALLOWED to be null is a design decision of each specific program; there is nothing inherently good or bad about having pointers be null. In fact, having a pointer be null is a standard way to signal that this variable is not (yet) set.

The "existing Evaluator" does NOT check for a "null string" by checking string length. It checks for a "zero-length string".

 

[Theraze]

This is exactly what you are doing? Saving the string, if the expression is in square brackets, or evaluating it as a number and leaving the string null otherwise? Good, if so, although your immediately following paragraph makes me wonder...

Ah, I was still setting it by initializing the values as "" in registerMonster, which would then call a new "Monster" with the "" value in the case of static mobs. I'll change that to not create a default "" anymore though and change the length checks to null checks.

The chaos I was referring to is if you have null and initialized strings mixed, and don't check for null at the start of every call, you'll often get a segfault. I somewhat recently converted a mud coded in C to compile/run with pedantic, Wall, and Werrors on, and then converted the memory handling to actually care about memory being released twice/not initialized properly. It took several months to find all the parts where it had been done poorly in the past, but it's been running fine and stable for a few months now and I feel better about it in general.

Anyways, moving back to Java and this, it doesn't currently check for the brackets. I got quite confused looking through Modifiers for where the bracket handling happens. What I used was... well, let me give an example.

    if ( option.equals( "HP:" ) )
    {
     if ( tokens.hasMoreTokens() )
     {
      value = tokens.nextToken();
      if (StringUtilities.isNumeric(value)) health = StringUtilities.parseInt( value );
      else evaluatehealth = value;
      continue;
     }
    }

The only change there is testing if the value is numeric. If so, it gets passed to parseInt. isNumeric is the same test used (in line 694) in parseIntInternal1 to decide if a string is cleaned up enough to run through parseInt.

If it would fail parseInt, there's no sense in trying to pass it on. Whether we bracket it or not, it'll fail. If we throw a test like value[0] == '[' into the saving of the expression, the question becomes what to do with expressions that fail that... do we want it to still try to evaluate them, or do we want them to void that monster?

Update: Tried sending uninitialized string, compile failed.

[javac] C:\kolmafia\src\net\sourceforge\kolmafia\persistence\MonsterDatabase.java:413: variable evaluatehealth might not have been initialized
[javac] return new Monster( name, health, attack, defense, initiative, attackElement, defenseElement, minMeat, maxMeat, poison, evaluatehealth, evaluateattack, evaluatedefense, evaluateinit );

So, do I go back to testing string length, do I construct a different form of Monster that has the strings, do I send the strings on each and evaluate it on creation...? Hmm, that probably wouldn't slow it down QUITE as much, but should fix the crashings...

Can't leave anything uninitialized, apparently...

[javac] C:\kolmafia\src\net\sourceforge\kolmafia\persistence\MonsterDatabase.java:477: variable evaluatehealth might not have been initialized
[javac] }

Can't leave uninitialized, but can initialize to null. This works. Or compiles, at least.

   if (StringUtilities.isNumeric(evaluatehealth)) { this.health = StringUtilities.parseInt( evaluatehealth ); this.evaluatehealth = null; }
   else { this.evaluatehealth = evaluatehealth; this.health = 0; }

 

[Veracity]

No, you cannot leave anything uninitialized.

	if ( option.equals( "HP:" ) )
	{
		if ( tokens.hasMoreTokens() )
		{
			value = tokens.nextToken();
			if (StringUtilities.isNumeric(value))
			{
				health = StringUtilities.parseInt( value );
				evaluateHealth = null;
			}
			else
			{
				health = 0;
				evaluatehealth = value;
			}
			continue;
		}
	}

When you want to look at the monster health, if evaluateHealth is != null, Eval it. Otherwise, use the constant value in health.

 

[Theraze]

Currently doing this, which should evaluate the same. For registerMonster, initializing health value:

  String evaluatehealth = null;

Reading health:

    if ( option.equals( "HP:" ) )
    {
     if ( tokens.hasMoreTokens() )
     {
      value = tokens.nextToken();
      evaluatehealth = value;
      continue;
     }
    }

And making the new monster.

  return new Monster( name, attackElement, defenseElement, minMeat, maxMeat, poison, evaluatehealth, evaluateattack, evaluatedefense, evaluateinit );

For creating a new monster:

   if (StringUtilities.isNumeric(evaluatehealth)) { this.health = StringUtilities.parseInt( evaluatehealth ); this.evaluatehealth = null; }
   else if (evaluatehealth == null) { this.health = 0; this.evaluatehealth = null; }
   else { this.health = 0; this.evaluatehealth = evaluatehealth; }

For reading health:

  public int getHP()
  {
   if ( this.evaluatehealth != null ) return (int) new MonsterEvaluator.Expression( evaluatehealth, "" ).eval();
   return this.health;
  }

 

[Theraze]

Scrap the register and creating parts above. Rethought it and am now doing the following, which should prove less... weird. For register:

  int health = 0;
  String evaluatehealth = null;

On evaluation:

    if ( option.equals( "HP:" ) )
    {
     if ( tokens.hasMoreTokens() )
     {
      value = tokens.nextToken();
      if (StringUtilities.isNumeric(value)) health = StringUtilities.parseInt( value );
      else evaluatehealth = value;
      continue;
     }
    }

And on creating the monster:

  return new Monster( name, health, attack, defense, initiative, attackElement, defenseElement, minMeat, maxMeat, poison, evaluatehealth, evaluateattack, evaluatedefense, evaluateinit );

When it actually creates the monster, it does the following:

   this.health = health;
   this.evaluatehealth = evaluatehealth;

Easy, sets the evaluative values to null if they aren't set, puts the constant values to 0 if they're unknown (which is the 'standard' initiative for most mobs, it appears... will need to put together an initiative update at some point), and... both compiles and runs, with displaying proper values for baron von rats.

Sidenote: statgain is based on attack value. Needed to tweak that as well to allow for monsters with varying 'base' attack.

 

[Theraze]

So yeah... my current issue is trying to completely rework the eval() function. I'm confused by the expect/until/optional/optional/expr/term/factor/value part. I don't see where they're getting used by the top in Expression or eval, though I know they have to be there somehow... I see that Expression has this.bytecode which calls expr(). Is that the only one that calls those private functions? Am I missing it? Sorry... I've put together the 3 files, the other 2 classes are extending the original Evaluator class. modifier_eval shows me that the ModifierEvaluator class is working as it should. The original code of the evaluate is what still needs to be redone.

This part is what still needs to be done before it has the slightest chance of being accepted, I think. It's the basic rewriting of the evaluation system.

I've implemented in Rinn's post, I'm getting scaling monsters providing their stats properly (at least, as much as has been scaled). Only bit that's wrong is the detuned radio... also, scaling monsters override post-scaling ML, right?

Coded a check getEvaluating that just looks to see if evaluatehealth, evaluateattack, evalutedefense, or evaluateinit is returning != NULL. If any of those match, it returns true. If all are false, it's a non-scaling monster. I'll hook those into the ML checks later.