ilpersi / BHBot

A bot that automates a game called Bit Heroes
GNU General Public License v3.0
28 stars 32 forks source link

Add edge-case for only having the first raid unlocked. Fix occasional… #99

Closed SohanBhuiyan closed 5 years ago

SohanBhuiyan commented 5 years ago

… Raid 'Clear' freeze

Currently if the user has only R1 unlocked and their setting has a raid level > R1, the program will print that it will do the highest raid possible but doesn't actually do anything. The problem lies in the fact that when only the first raid is unlocked, there no circle level selector (cueRaidLevel/ cueRaidLevelEmpty) available. Since setRaidType returns null in that case, I use that to indicate the player only has R1, and we will run R1 instead of showing an error.

Sometimes while doing a raid, the program will execute the weekly reward condition which mess with processDungeon() and makes it miss the 'Clear' cue detection. This was easily solved by placing the condition which checks different states (state == State.Raid || state== State.trials ||....) above the news and weekly reward conditions.

ilpersi commented 5 years ago

Hi, and first of all thank you for taking time to send us a PR!

I have a couple of question:

1 - Why did you move the state check block? Does it impact in any way the raid issue you're reporting? Ideally, that check should stay where it is now just before entering the main menu handling.

// process dungeons of any kind (if we are in any):
if (state == State.Raid || state == State.Trials || state == State.Gauntlet || state == State.Dungeon || state == State.PVP || state == State.GVG || state == State.Invasion || state == State.UnidentifiedDungeon || state == State.Expedition || state == State.WorldBoss) {
    processDungeon();
    continue;
}

2 - Can you please use the shot onlyR1 command and share with us the resulting screenshot. Be sure to hide sensitive information, but please do not crop/cut/resize the image in any way as otherwise we can not use it. This will help us to understand if there's a better way to manage the "only R1" edge case.

Thank you again.

SohanBhuiyan commented 5 years ago

Hey, so after reading your comment and testing my changes again here is what I have come to.

1 - The amount of time the bot gets stuck at the 'clear' screen for raids is pretty inconsistent with the current release, at least for me. When I moved the conditional that you had in question to the top I did not have any 'clear' freezes anymore. Since this problem doesn't seem that frequent, I say not to pull in that change.

2- As requested, I have attached the onlyR1 screenshot using the command, and I also attached a screenshot of the console during that execution.

onlyR1_20190808

onlyR1Logs

ilpersi commented 5 years ago

I've committed a simpler fix in the master branch, let me know if it works for you.

SohanBhuiyan commented 5 years ago

So your commit did correct the right log information to be printed. Before it said the highest raid that it will do is R0 instead of R1.

Screenshot_7

However, it did not fix the part about actually doing the raid itself. It will just stay on the main raid screen. The problem lies here

      if (!setRaidType(raidType, raidUnlocked)) {
             BHBot.logger.error("Impossible to set the raid type!");
             continue;
        }

setRaidType returns false in this case because it cannot detect cueRaidLevel. (Remember, users with only R1 unlocked does not have those cues unlocked yet).

An easy fix is to comment/remove the continue statement so that the program can still continue onto the rest of the raid code where it detects the summon cue and perform a regular raid.

Fortigate commented 5 years ago

I've updated setRaidType to return true if R1 is input as the unlocked raid, and to continue running that way. This is to keep the error message and continue as a failsafe in case we encounter other issues changing raids.

This /should/ fix it once and for all! Let us know

ilpersi commented 5 years ago

@Fortigate In my opinion the last fix is not correct as it introduces a regression . If I want to do R2 and I have R1 selected (for whatever reason) setRaidType will return true.

Basically if R1 is selected, with the current code, the bot will never change it.

Fortigate commented 5 years ago

setRaidType() takes the unlocked raid tier, not the selected raid tier as the second input so that's not an issue.

However we can't currently changed from R1 to R7 due to the 26px offset on raid buttons being inaccurate when the distance is 5-6 tiers (same issue we had reading current raid when R6 was released). I'll add a new function returning the correct points similar to WB.

EDIT: Ignore that, I renamed the variables to make it clearer and see your point. Working on changing the system to the dungeon screen moving system.

Fortigate commented 5 years ago

More fixes added in 95df6f580878679a41d3ba6814f87bc5a7e89e97. I removed the R1 check in setRaidType and moved it to skip attempting to change the raid if only R1 is unlocked.