loathers / autoscend

An ascension script for KoLMafia
Other
45 stars 67 forks source link

auto_mcd_target does not respect auto_MLSafetyLimit #598

Closed chunkinaround closed 3 years ago

chunkinaround commented 3 years ago

Prerequisites

Please answer the following questions for yourself before submitting an issue. YOU MAY DELETE THE PREREQUISITES SECTION.

Context

Expected/Desired Behavior

auto_MLSafetyLimit includes ML from MCD

Current Behavior

What is the current behavior? Currently MCD settings will not take into account auto_MLSafetyLimit

Failure Information (for bugs)

As reported by multiple users in the discord auto_MLSafetyLimit is not taken into account when setting the MCD. in the example log below you can see that auto_mcd_target is being set outside either pre or post adv scripts:

[INFO] - Post Adventure done, beep.
[INFO] - Turn(393): Starting with 6 left at Level: 7
[INFO] - Encounter: -10.0 Exp Bonus: 5.050000000000001
[INFO] - Meat Drop: 70.0 Item Drop: 0.0
[INFO] - HP: 43/44, MP: 35/35, Meat: 6157
[INFO] - Tummy: 15/15 Liver: 14/14 Spleen: 0/15
[INFO] - ML: 0 control: 0
[INFO] - Delay between adventures... beep boop... 
Countdown: 1 second...
Waiting completed.
Preference _auto_thisLoopHandleFamiliar changed from true to false
Preference auto_familiarChoice changed from Frumious Bandersnatch to
Preference auto_maximize_current changed from 5item,meat,0.5initiative,0.1da 1000max,dr,0.5all res,1.5mainstat,mox,-fumble,0.4hp,0.2mp 1000max,3mp regen,1.5weapon damage,-0.75weapon damage percent,1.5elemental damage,2familiar weight,10exp,5Moxie experience percent,-200combat 25max,ml -150max,effective to 5item,meat,0.5initiative,0.1da 1000max,dr,0.5all res,1.5mainstat,mox,-fumble,0.4hp,0.2mp 1000max,3mp regen,1.5weapon damage,-0.75weapon damage percent,1.5elemental damage,2familiar weight,10exp,5Moxie experience percent
Preference auto_mcd_target changed from 0 to 11
[INFO] - Attempting to make the Hidden Temple less hidden.
Preference auto_diag_round changed from 2 to 0

[INFO] - Starting preadventure script...

That is because we are setting the ML as part of the core do_tasks loop in main autoscend with the function basicAdjustML();

This changes the variable auto_mcd_target. Finally auto_change_mcd will check what the setting should be, which has been set to 11.

There are a couple of options for resolving this here. We can introduce a change in basicAdjustML(), auto_change_mcd(), auto_pre_adventure(), or enforceMLInPreAdv(), actually we could introduce changes in tons of places, thats why Im creating an issue not a PR right away

If it is not a bug, please remove the rest of this template.

Failure Logs

Please include any relevant log snippets or files here. You should still attach the full KoLMafia session log if possible.

chunkinaround commented 3 years ago

In general, what is the purpose of running basicAdjustML() in the doTasks() loop when we have handling in both pre and post adv, isnt it better to let that handling exist there?

jeparo commented 3 years ago

In general, what is the purpose of running basicAdjustML() in the doTasks() loop when we have handling in both pre and post adv, isnt it better to let that handling exist there?

Anyway, basicAdjustML should definitely take into account that setting (perhaps just calling auto_setMCDToCap(). Thank you for finding this.

My understanding (and this might be wrong, it's been a year since I've touched this code), is

  1. basicAdjustML is for location-independent behavior
  2. pre-adv ML is for configuring location-specific ML adjustments: e.g. we want to run very little ML in the Logging camp, but very high ML on oil peak. Critically, this is done AFTER we choose where to adventure, which is awkward because...
  3. post-adv ML is only done in Ed, which maintains the Blessing of Seqet. I might be missing something. This is kinda weird, lol. Maybe it's fine in Ed because we know that the floor for Ed runs can handle this safely? shrug

If we ripped out basicAdjustML entirely, there might be some suboptimal behavior here where (for example) our willingness to complete a quest depends on our ML. I'm not sure how big a deal this actually is (probably very minor!), but there are a few places in the codebase where we seem to set/unset/reset values every loop, and this is why. It's hard to know if you're breaking something. :(

taltamir commented 3 years ago

IIRC there is something broken in one of those functions because it was assuming you would first do an MCD 0 command before you run it or some such, which we removed some time ago without replacing.

We need some serious refactoring on how ML is handled. The numerical safety limit is not a really good way to handle "I keep on dying, stop commit suicide due to high ML" issue with autoscend

I think what we need:

  1. a function that predicts our ability to survive a zone in which we are about to adventure, to make sure we do delay zones that are too tough.
  2. consolidate most ML functions into a single pre-adventure function that selects an appropriate ML for the target location based on our situation and the target location (similar to our noncombat adventure file).
  3. allow for path specific exceptions to number 2.. which would be done via functions called from the function at number 2
  4. auto_MLSafetyLimit should be replaced by a cleaner override for manipulating ML. its stated goal is better achieved by a directive to just be more cautious on ML than a digit. Or possibly even removed if we have number 1 implemented

I have been considering getting around to working on ML it for some time now