Feature Patch: write maximizer suggestions to CLI output
- Thread starter jonchang
- Start date
Without looking at any code or patch I think I would much refer extending the dump parameter. That way those of us who don't want/need to see speculation results in the gCLI can opt out merely by not running with dump.
My recollection is that maximizer string in the gCLI was there because the string actually submitted to the maximizer was needed for debugging. The results of submitting that string were not nearly as important for debugging as the string itself.
Running headless has never been a KoLmafia implementation goal - more of something that can be supported. It might be better to ask what the output for maximize and maximize? should be when headless and make the changes only when running headless?
My recollection is that maximizer string in the gCLI was there because the string actually submitted to the maximizer was needed for debugging. The results of submitting that string were not nearly as important for debugging as the string itself.
Running headless has never been a KoLmafia implementation goal - more of something that can be supported. It might be better to ask what the output for maximize and maximize? should be when headless and make the changes only when running headless?
Thanks for the quick responses, but could I perhaps get a review based on having seen the patch? I attached it in my original post, but here it is again:
Code:
Index: src/net/sourceforge/kolmafia/textui/command/ModifierMaximizeCommand.java
===================================================================
--- src/net/sourceforge/kolmafia/textui/command/ModifierMaximizeCommand.java (revision 20548)
+++ src/net/sourceforge/kolmafia/textui/command/ModifierMaximizeCommand.java (working copy)
@@ -33,11 +33,14 @@
package net.sourceforge.kolmafia.textui.command;
+import java.util.Iterator;
+
import net.sourceforge.kolmafia.KoLConstants.MafiaState;
import net.sourceforge.kolmafia.KoLmafia;
import net.sourceforge.kolmafia.KoLmafiaCLI;
import net.sourceforge.kolmafia.RequestLogger;
+import net.sourceforge.kolmafia.maximizer.Boost;
import net.sourceforge.kolmafia.maximizer.Maximizer;
public class ModifierMaximizeCommand
@@ -58,8 +61,23 @@
RequestLogger.updateSessionLog();
RequestLogger.updateSessionLog( command + " " + parameters );
}
+
+ boolean success = Maximizer.maximize( parameters, 0, 0, isSpeculateOnly );
+
+ if ( isSpeculateOnly )
+ {
+ Iterator i = Maximizer.boosts.iterator();
+ while ( i.hasNext() )
+ {
+ Object boost = i.next();
+ if ( boost instanceof Boost )
+ {
+ RequestLogger.printLine( boost.toString() );
+ }
+ }
+ }
- if ( !Maximizer.maximize( parameters, 0, 0, isSpeculateOnly ) && !isSpeculateOnly )
+ if ( !success && !isSpeculateOnly )
{
KoLmafia.updateDisplay( MafiaState.ERROR, "Unable to meet all requirements via equipment changes." );
RequestLogger.printLine( "See the Modifier Maximizer for further suggestions." );The output of
maximize is unchanged, since if you're changing equipment mafia will tell you what you're doing regardless.From my point of view,
maximize? is not currently very useful as all it does is tell you a score and whether the maximize failed or not. This adds more information. Based on my experience with using mafia, it seems to lean towards being more verbose rather than being more terse.Finally, dump seems to be doing something quite different as it seems to be for debugging what items are being considered and how ties are broken, rather than providing information to the typist at the CLI for making equipment or effect changes.
This output is still available even after the patch.
I don't see what running headless has to do with it, since I don't run headless and it's a poor user experience regardless due to assumptions about the consumer of the CLI being able to render HTML. I can always click over to the maximizer panel, but when I'm already in the CLI it's easier to just type out the maximizer string I want and see what the maximizer panel would propose anyway.
I could do that, but I prefer to patch mafia and add a feature for everyone.
It works. It ignores the maximizer filters (which is also true when the command is used from the gCLI). The patch only changes the gCLI command, no changes to maximizer, so no need to test a lot of strings.
If you'd like to creep up the scope a bit, I think
maximize? <blah> should work just like maximize <blah>. Either that, or there should be a way to set those filters and prefs, and I haven't thought up with a good way to do that without making a really complex command line much more complex. ash may be the best way.also, please patch the maximizer-help.txt file to reflect your new changes.maximizer-help.txt said:
So, a verbose flag for maximize? would give you what you want.
Last edited:
on second look, it doesn't fail if the result is (FAIL). I specifically named an item I don't have, and I expected it to fail.
Code:
> maximize? mox, +equip swordzall
Maximizer: mox, +equip swordzall
Maximizing...
448 combinations checked, best score 89.00 (FAIL)
equip hat mariachi hat (+1)
equip weapon aerogel accordion (+20)
equip off-hand SalesCo sample kit (+0)
keep back: vampyric cloake
keep shirt: astral shirt
equip pants liar's pants (+5)
equip acc1 batskin belt (+3)
keep acc2: hewn moon-rune spoon
keep acc3: Retrospecs
keep familiar: pile of dungeon junk
synthesize Synthesis: Cool (1 spleen, +99) [30 advs duration, 15 uses remaining]
spacegate vaccine 2 (+17) [30 advs duration, 1 use remaining]
use 1 flask of porquoise juice (+17) [10 advs duration, 1 in inventory]
cast 1 The Moxious Madrigal (2 mp, +10) [10 advs duration]
cast 1 Bind Penne Dreadful (150 mp, +10)
cast 1 Disco Smirk (10 mp, +10) [10 advs duration]
cast 1 Quiet Desperation (10 mp, +8) [10 advs duration]
buy & use 5 hair spray (22 meat, +5) [3 advs duration]
cast 1 Blubber Up (7 mp, +5) [10 advs duration]
horsery crazy horse (+4) [0 meat]
cast 1 Stevedave's Shanty of Superiority (30 mp, +3) [10 advs duration]
use 1 candy harmonica (+3) [100 advs duration, 1 in inventory]
cast 1 Disco Fever (10 mp, +3) [10 advs duration]
monorail buff (+3) [10 advs duration, 1 use remaining]
cast 1 Disco Aerobics (1 mp, +2) [5 advs duration]
cast 1 Moxie of the Mariachi (1 mp, +1) [5 advs duration]Since
maximize actually changes equipment, and that mafia already reports these equipment changes, I felt it unnecessary to also report why it was changing that equipment. Note that since maximize is used extensively from e.g. ash scripts and other devs have already complained about potential problems with verbosity I'm not inclined to make this change given how much resistance there's been to this already.One idea would be to make
maximize from the CLI respect the filters set up from the GUI maximizer panel. I haven't looked into what that would require, but would be interested in working on it as a follow up to this patch.Sure, but I don't think maximize? is currently very useful since it provides next to no information. And I want to point out again that the mafia CLI is already really verbose, and that this change only affects people who use maximize? at the CLI. I checked all of the ash scripts I have installed and none of them use
maximize?. I also haven't seen any CLI commands that include a verbose flag, so if you could point one out to me that we could model from that would be great.This is the same as the current behavior, which dates back to r8954 in 2011.
I've attached an updated patch.
Last edited:
Ah, I didn't mean "show things for
maximize". To be more clear, I was suggesting that maximize? from the command line should limit itself to equipment on-hand and didn't check prices. Not sure if it's the right behavior, but it seems odd to me that the two commands act so differently. I think it has to do with the UI mutating over time, but not the command line, but it doesn't matter why.I think that would be a good follow-up. You might also want to set and get via the cLI, which also leads to saving those values. The latter was always a long-term "maybe I should ..." for the Maximizer GUI.
I guess what I'm thinking is I don't know how other people are using it, so changing the behavior without a way to keep existing behavior raises concerns. What I'd like to see is some way to have your cake and eat it, too. A preference would be one way. A new flag is something we've already mentioned. Or a separate command. I am suggesting extending the commands rather than altering them.
I have done my share of "no one could possibly mind this change...", and been wrong, so I try to remember this XKCD cartoon ...
Yeah, at this point we might just document that it does not fail if it cannot fulfill
+equip <X>The reasoning is already documented:
This change to maximize? preserves the reasoning behind this since it permits the user to choose which effects to apply, rather than just excluding them from consideration altogether.
I've attached an updated patch that documents this.