29988122 / Fate-Grand-Order_Lua

Fate Grand Order auto battle script - no root needed, for Android use only
MIT License
291 stars 95 forks source link

Discussion with my collaborator #21

Open 29988122 opened 6 years ago

29988122 commented 6 years ago

Having a new discussion thread seemed like the fastest way for us to discuss non-commit and non-PR related issue. @ryuga93

I can't figure out the function calling here:

function menu()
    atkround = 1
    npClicked = 0
    turnCounter = {0, 0, 0, 0, 0}
    click(Location(1900,400))
    wait(1.5)
    if Refill_or_Not == 1 and stoneused < How_Many then
        refillstamina()
    end
    click(Location(1900,500))
    wait(1.5)
    click(Location(2400,1350))
    wait(8)
end

function refillstamina()
    if StaminaRegion:exists("stamina.png", 0) then
        if Use_Stone == 1 then
            click(StoneClick)
            click(Location(1650,1120))
            stoneused = stoneused + 1
        else
            click(AppleClick)
            click(Location(1650,1120))
            stoneused = stoneused + 1
        end
    wait(1.5)
    menu()
    end
end

The 3rd line from last memu() called inside refillstamina() was the one you recently added. Wouldn't it caused a lot of wait(1.5)?

1.Every time when refillstamina() was called but game's not currently at AP refill window. 2.Every time memu() was called).

Or I got the whole function calling inside function concept wrong?

ryuga93 commented 6 years ago

The wait & menu() in refillstamina() will not be called if stamina.png does not exist.

I think I forgot to indent both functions...

29988122 commented 6 years ago

sure I understand, but then it will keep calling this part

click(Location(1900,400))
 if Refill_or_Not == 1 and stoneused < How_Many then
        refillstamina()
    end

And misbehave, right? Just tested that.

If I did the permission right, you should be able to commit freely now(collaborator thing). Make changes as you wish! : )

ryuga93 commented 6 years ago

Once it click on the OK and refilled the stamina, the game will return to the home window, at least for EN.

I change this because for me the script lack one step and stop at friend selection window.

Will it move to friend selection window after refill for JP?

29988122 commented 6 years ago

Yes it will

Wow, version differences....... We should make an extra variable to bypass this issue.

I guess only in JP it will move to friend selection directly, which is probably NOT the case in other countries. Because I played the game since 1st day, so I remembered this behavior change.

29988122 commented 6 years ago

https://github.com/29988122/Fate-Grand-Order_Lua/commit/ca56a42245ec4f18398d1c65c7cbc918e5ed99fa Put a temp variable to solve the issue. Added a few comments to code. Also fixed the timing to be more forgiving..since I ran Memu and it's not a good thing to have timing too tight - when I was playing overwatch, it stuck frequently lol. Bad PC.

ryuga93 commented 6 years ago

I can't even run Memu with Dota :/

29988122 commented 6 years ago

QQ

Anyway from now on I shall focus on JP version, you can test it on EN version only. I guess ppl will file an issue whenever bug occurred.

Now I understand why ppl said Unit test / test automation is important. Such a hassle to test and document everything.

ryuga93 commented 6 years ago

Yea, and the reason I push the updates slowly, first the card selection, then the autoskill.

That would be a mess if I push all at once and a bug decided to say hello.

I'm from EE background, so initially my coding was way messy that yours. A software engineering job totally changed my coding: modular functions, minimize repetition, unit test etc...

Btw, not sure you realize or not, I haven't added the combat uniform switch skill support. That will come later :P. Also, a collaborator for CN would be a big plus for the long run. My experience with CN games tells me that the road will be "坎坷辛苦" @@

29988122 commented 6 years ago

Really have to thank your for the autoskill.

The 2018 Vaneltine's is significantly harder than other event..it helped a ton : p

ryuga93 commented 6 years ago

I got bride Nero for the reward XD

ryuga93 commented 6 years ago

新年快乐!

29988122 commented 6 years ago

新年快樂 : D

29988122 commented 6 years ago

Recently FGO CN changed their font, again. Damn the lack of support of transparent images is such a hassle.

About the recognition issue of current stage status, I've tried various methods included:

  1. http://ankulua-tw.boards.net/thread/10#numberOCR Not working, returned strange value(66) rather than throwing exception or correct result.

  2. Sikuli is using Tesseract to do OCR, but this is not provided by Ankulua. A no-go.

  3. Figure out how Sikuli implemented find(), and try to apply a dirty processing onto stage pngs for them to be recognized. Not working because I know 0 shit.

For now I just throw in a http://sikulix-2014.readthedocs.io/en/latest/pattern.html#Pattern.similar , hope it's going to work.

Any ideas?

29988122 commented 6 years ago

Well in the end I decided to use simple snapshot diff to determine the change of stages.. The only side effects is that you'll have to start from battle 1/3 and cannot pause in the middle of battle.

ryuga93 commented 6 years ago

Hi, sorry for not checking in, how is the autoskill going? My pc broke down, and I'm not able to contribute much due to work. But I'll try to get to you if you need to discuss stuff.

29988122 commented 6 years ago

It's fine, we all have our work/life/side project/hobby balances. : D

I implemented a way to save region snapshot every turn, and use it as the base pic to be compared next turn, therefore less font issue.

Worked fine currently, unless the background's #0,0,0 xD

29988122 commented 6 years ago

Heard that US server has a slight UI upgrade. Remember to check if anything's abnormal, thx bro!

29988122 commented 6 years ago

I have no words @ryuga93 @potchy

https://github.com/29988122/Fate-Grand-Order_Lua/commit/42ae17b1d9be4fb97992958d32f64d3e00d3dc88#diff-1b3d9652b77d835d34d699a6f005f563

They just reuse last year's code, and thus the stamina refill behavior was also from one year ago...

廢到笑。

ryuga93 commented 6 years ago

So the US now will directly enters friend selection after refill?

Edit: Oh it's the new jp event. Well they know the DRY concept in programming XD

29988122 commented 5 years ago

@ryuga93 @potchy

Thanks guys, I appreciated everything you've done to the script. Version number is 2.0.0 now. Readme.md is now edited to give credit to you guys.

Live long and prosper.

ryuga93 commented 5 years ago

Yay ver 2.0! Thanks @potchy!

29988122 commented 5 years ago

@potchy To be honest I don't really understanding the support module. I'm writing docs XD

Am I correct for the following behavior? Please correct me line by line, therefore I can have a better grasp of the code.

Settings:
preferred servants: "waver, merlin, caster fox"
preferred CEs: "lunchtime, any, lunchtime"
Support_SwapsPerRefresh = 3
Support_MaxRefreshes = 2
Then:
It will check the topmost *3* entries on the support list. 
It will check for the support-CE *PAIR*, e.g., waver with lunchtime, merlin with any CE, caster fox with lunchtime.
It will check the support-CE pair recursively from the 1st entry, then 2nd, and then 3rd entry on the support list.

Is the above description correct?

Another question: How about "any" pair? e.g.,

preferred servants: "waver, caster fox, any"
preferred CEs: "lunchtime, lunchtime, any"

Then it will search the 1st entry on the friend support list(any, any pair worked as a fallback), no matter the value of Support_SwapsPerRefresh is. Right?

29988122 commented 5 years ago

Yet another question.

in regular.lua, line 285~308, function TargetChoose(): I don't really understand the return value, return function, and the structure of ankulua-utils.lua.

Without changing its behavior(guess I'll change them later), how would you rewrite TargetChoose() using the method you've been calling in ankulua-utils.lua?

potchy commented 5 years ago

Eh... no. That is not how it works at all. And I tried my best to write clear code. 😢


Firstly, you can't write Any more than once. I use it mostly in events, as I don't care which Servant the script selects as long as it has a Bonus CE equipped like Maid in Halloween.

So, if you write this:

Support_PreferredServants = "Any"
Support_PreferredCEs = "lunchtime.png, maid_in_halloween.png"

or this:

Support_PreferredServants = "" -- leaving it blank works the same way
Support_PreferredCEs = "lunchtime.png, maid_in_halloween.png"

Then the script will search for: • Chaldea Lunchtime • OR Maid in Halloween


And if you write this:

Support_PreferredServants = "waver.png, tamamo.png"
Support_PreferredCEs = "Any"

or this:

Support_PreferredServants = "waver.png, tamamo.png"
Support_PreferredCEs = ""

Then the script will search for: • Waver • OR Tamamo


Finally, if you write this:

Support_PreferredServants = "waver.png, tamamo.png"
Support_PreferredCEs = "lunchtime.png, maid_in_halloween.png"

Then the script will search for: • Waver + Lunchtime • OR Waver + Maid in Halloween • OR Tamamo + Lunchtime • OR Tamamo + Maid in Halloween

potchy commented 5 years ago

As for ankulua-utils...

local function useSameSnapIn(func)
    snapshot()
    local value = func()
    usePreviousSnap(false)

    return value
end

Here, func is an actual function passed by argument. It may or may not produce a value. We do this all the time in Javascript. Initially, I wrote it like this:

local function useSameSnapIn(func)
    snapshot()
    func()
    usePreviousSnap(false)
end

However, in support.lua:97, specifically, I needed the return value of searchMethod, which is the matched Servant/CE. So, I made useSameSnapIn store the value produced by func and return it later.

In regular.lua, I would rewrite battle() like so:

function battle()
    ankuluaUtils.useSameSnapIn(battlePartOne)

    wait(0.5)
    if decodeSkill_NPCasting == 0 then
        --enter card selection screen
        click(Location(2300,1200))
        wait(1)
    end

    if Battle_NoblePhantasm == "spam" or (Battle_NoblePhantasm == "danger" and TargetChoosen == 1) then
        ultcard()
    end

    wait(0.5)
    ankuluaUtils.useSameSnapIn(doBattleLogic)

    atkround = atkround + 1

    if UnstableFastSkipDeadAnimation == 1 then
        --https://github.com/29988122/Fate-Grand-Order_Lua/issues/55 Experimental
        for i = 1, 6 do
            click(Location(1500,500))
            wait(2)
        end
    end
    wait(3)
end

function battlePartOne() -- please don't make me decide the name :C
    wait(2.5)
    InitForCheckCurrentStage()

    --TBD: counter not used, will replace atkround.
    local RoundCounter = 1

    if TargetChoosen ~= 1 then
        --Choose priority target for NP spam and focuse fire.
        TargetChoose()
    end

    wait(0.5)
    if Enable_Autoskill == 1 then
        executeSkill()
    end
end

Obviously, I would also remove all calls to usePreviousSnap. I hope I explained myself well. :flushed:

29988122 commented 5 years ago

Eh... no. That is not how it works at all. And I tried my best to write clear code.

Apparently it's my problem - a n00b. XD

Anyway it's clear enough now! Thanks for the tutoring, my Mr. Mentor : D

Will modify the readme and comments according to this behavior.

29988122 commented 5 years ago

Obviously, I would also remove all calls to usePreviousSnap. I hope I explained myself well

It gave me chills when I read my old comment

--TBD: counter not used, will replace atkround.

Yeah, refactoring incoming! Gotta read more coding style related book.

potchy commented 5 years ago

No prob. I'm still learning Lua as well. I'm just lucky that it's very similar to Javascript. 😅

potchy commented 5 years ago

So... @ryuga93, about AutoSkill, why did you choose to subtract 96 from the ASCII code?

function decodeSkill(str, isFirstSkill)
    --magic number - check ascii code, a == 97. http://www.asciitable.com/
    local index = string.byte(str) - 96`

Couldn't this line

if index >= -44 and index <= -42 and decodeSkill_NPCasting == 0 then

have been written like this instead?

if index >= 52 and index <= 54 and decodeSkill_NPCasting == 0 then
ryuga93 commented 5 years ago

It's an obfuscation to prevent others cloning our script and make it as their own!

Just kidding XD

SkillClickArray = {Skill1Click, Skill2Click, Skill3Click, Skill4Click, Skill5Click, Skill6Click, Skill7Click, Skill8Click, Skill9Click, Master1Click, Master2Click, Master3Click}

I tend to make the code a bit complicated while making it cleaner...

SkillClickArray[97] = Skill1Click
SkillClickArray[98] = Skill2Click
.
.
.

If the subtraction does not make the script run slower significantly, 1 line is just nicer than 12 lines lol

potchy commented 5 years ago

Oh, right. I totally missed that. :expressionless: That's pretty smart, actually, but I was thinking of changing those declarations to something a bit more explicit, as to make it easier for newcomers. What do you think?

From this:

SkillClickArray = {Skill1Click, Skill2Click, Skill3Click, Skill4Click, Skill5Click, Skill6Click, Skill7Click, Skill8Click, Skill9Click, Master1Click, Master2Click, Master3Click}
SkillClickArray[-47] = Servant1Click
SkillClickArray[-46] = Servant2Click
SkillClickArray[-45] = Servant3Click
SkillClickArray[-44] = Ultcard1Click
SkillClickArray[-43] = Ultcard2Click
SkillClickArray[-42] = Ultcard3Click

To this:

SkillClickTable = {
    -- 1st Servant skills
    ["a"] = Skill1Click,
    ["b"] = Skill2Click,
    ["c"] = Skill3Click,

    -- 2nd Servant skills
    ["d"] = Skill4Click,
    ["e"] = Skill5Click,
    ["f"] = Skill6Click,

    -- 3rd Servant skill
    ["g"] = Skill7Click,
    ["h"] = Skill8Click,
    ["i"] = Skill9Click,

    -- Master skills
    ["j"] = Master1Click,
    ["k"] = Master2Click,
    ["l"] = Master3Click,

    -- Skill targets
    ["1"] = Servant1Click,
    ["2"] = Servant2Click,
    ["3"] = Servant3Click,

    -- Noble Phantasms
    ["4"] = Ultcard1Click,
    ["5"] = Ultcard2Click,
    ["6"] = Ultcard3Click
}

Sure, there are more lines, but I believe it's more intuitive this way. Plus, it will be easier to introduce new characters for target change (#32) if we ever decide to do it lol

ryuga93 commented 5 years ago

Sure! I agree to make the code intuitive for community project.

Coming from C/Java background, all I know about arrays is variable[index] = value

I was quite surprise to discover that lua accepts negative index when I first coded the autoskill, due to it's table/key-value nature.

29988122 commented 5 years ago

Me too.

When I read this one: https://www.lua.org/pil/11.html

I was like MIND=BLOWN

Also why the f the index started from 1 lul

29988122 commented 5 years ago

Guess I need somebody's help make the site running. Time's scarce to me at the moment.

Checked Github hosting, and apparently, I have 0 idea how to do frontend/jekyll/wordpress/even the simplest template applying.

The site will include dev info(TBD, mentioned by ryuga93), contributor info, and i18n of readme.md. Also, current contributors ( @ryuga93 @potchy @TryBane ) must be assigned repo permission.

TODO: I don't seem to get the terminology between projects, organizations, and personal repo. This needs to be solved before hosting our site repo, I guess. Currently /Fate-Grand-Order_Lua's hosting as my personal repo on Github.

Meanwhile,

  1. I'll overhaul the current readme to its smallest possible form i.e., user information ONLY, increase the readability. Other behavior description/code details will be in the abovementioned site.

  2. Also, by reducing the amount of comments in the setting Lua files, we won't have to do minor trival comment changes in the future.

  3. By redirecting user to the readme/githubpage site, we're able to separate the idea of "comments" and "documentations" once and for all. We've just done refactoring, and I believe it's the correct timing doing so to eliminate all the hassle in the foreseeable future.

potchy commented 5 years ago

I feel like having a separate website for such a small project is kinda pointless. If it's just for the sake of having a better organized documentation, it's easier to move everything to the Wiki tab instead.

TryBane commented 5 years ago

Reading through here made me realize how funny it was that I implemented the change to pure chars for indexes when potchy proposed it 2 months ago. Also might be a good idea to comment their representations like he did, and I'm considering doing just that. Also would like to move the toast that appears at the main menu portion of the script to the init() function as it does not need to be repeated every iteration (I have never had an instance where I needed to be reminded that it will change targets while it was running, since I never kept watch outside of testing functionality after a change)

Also, there is still a lot of refactoring that needs to be done. I am working on creating a new module to handle the observer method. Also adding additional functions to handle some of the messier things in the code.

But before that, if @potchy wants to take care or the resolution scaling, it should not conflict with the observer module I am working on.

Also not sure if this was what you intended, but I am not fond of the idea of removing comments from the modules or regular.lua. I can understand removing it from the settings file however, as it might make it easier for users to deal with.

29988122 commented 5 years ago

@potchy Okay, I somehow ignored the wiki tab totally. My bad LMAO.

Will cope.

Better Workflow

@ryuga93 @potchy @TryBane

First, we should NOT and will NOT change out current workflow just for the sake of a new, fashion methodology. It should stay the same 99% of the time.

To us, this "project" section is just a better version of a "to-do notepad"! It also helps visualize our current pending bugs/wait-to-be-done enhancements. Since issue tracker can be a little bit overwhelming.

That's all. Again, our workflow should stay the same - take it easy.

I write a simple readme in the description sections. https://github.com/29988122/Fate-Grand-Order_Lua/projects

TryBane commented 5 years ago

Ah, I see what you're talking about. I haven't used github for collaboration in a long time (Jan of 2017) so I didn't even know that was a thing.

I also think that is a good idea, centralize the known bugs/enhancement ideas. Since the issues can be opened by anyone some can/will be dupes of already requested enhancements. I like this idea, and I think it will help to keep track.

Might also be able to just have someone mark that they are working on it so that others don't think it is being left alone. Just a thought.

29988122 commented 5 years ago

@TryBane Yes. We don't often "freeze" because you know, our codebase should have some level of abstraction.(hint: we don't have that 1 month ago)

We(regular user included) should be able to make changes as the form of PR all the time(avoid commit only unless it's a simple tweak that doesn't need discussion), that's how version control system - git - works.

See this picture for example of your said issue: default

It's now with more clarity.

TryBane commented 5 years ago

Yes, I consider what potchy has done more as refactoring than what I did. Some of the things i did was refactoring, but not nearly everything I wanted. I felt that getting the new system for autoskill up was more important to get right first, then work on refactoring. I think I will be able to do that now, even while potchy works on the resolution scaling algorithm. As for Ryuga enhancing autoskill, I believe that might wait till I've finished the observer module. Mostly because the changes he would make might affect the way observer is written. Though if I discuss my intentions first, it will become clear whether that is necessary.

29988122 commented 5 years ago

Simple workflow example for you to follow from now on (or NOT, I'll do the chores for you every now and then, this is my responsibility xD):

  1. I create a new issue as what I said in this thread https://github.com/29988122/Fate-Grand-Order_Lua/issues/142

  2. I classify the issue into BOTH label: enhancement and project: enhancement.

  3. I then enter project section, move the said issue/card from "request (unassigned)" to "somebody's doing it"

  4. Finally, I add a note under that issue card telling others the current status of that issue.

default

So yeah, by doing so we can better understand the current status of this repo.

TryBane commented 5 years ago

Makes sense to me. I think this sounds like a decent way to approach the workflow. So you would be the only one moving the enhancements over or are we free to do so ourselves and then mark our name below? And if not, where do we claim an enhancement to work on, here or somewhere else?

29988122 commented 5 years ago

@TryBane

free to do so

I tend to AVOID interrupt our current interactive flow - we're doing this for pure pleasure, rules will ruin it(probably). So do literally anything as you please.

Currently we discuss things in many separated issue/PR/commit, right? When we feel like we have consensus in the discussion, we draft a PR / commit, right? Keep it that way, feel no pressure.

I just want to provide an extra way to track our discussion. You can help creating issues->projects->notes if you remember/feel happy (not boring) doing so. However, you can also do the codes/discussions only, as always; I'm the one who's responsible to put them into our to-do notebook for you to track - smooth our collaboration process.

29988122 commented 5 years ago

@TryBane >As for Ryuga enhancing autoskill, I believe that might wait till I've finished the observer module.

So I add a note into project section, am I right about the description? default

TryBane commented 5 years ago

Yeah, that looks accurate to me. Also just found out how to get there on mobile. Not as intuitive as I'd like but that is githubs issue. I think this will be a nice at a glance view of what is being worked on. Hopefully nobody will take issue if I work on a bunch of the ones not claimed, since I enjoy programming and always look for an excuse to do so.

TryBane commented 5 years ago

Regarding the Readme changes you've made so far @29988122 (and maybe you aren't done with them yet) but just wanted to point out that Capturing images for Screen Recognition should probably suggest that the steps are usable for adding Servant/CE images for supports and others. Just a thought since I've seen some asking for help regarding that. Might even be a good idea to link that specific location in the support selection area so they can easily see there is step-by-step instructions on how to do just that.

Also, the isEvent option is not for (or at least not anymore as far as I'm aware) an extra point reward screen at the end of the result menu. It is for quest item buffs such as what you see in Onigashima or Halloween 2016 (Halloween 2018 for EN) events where there were special items you had the option of using at the beginning of any mission. It may have been for a point reward system at the end of result screen before, but the system I implemented should take care of that too (though I may need to make it slightly longer for that and another reason)

potchy commented 5 years ago

Uh... how would guys call a module that contains only Locations/Regions? ui, gui, game-map, map? My creativity is lacking.

TryBane commented 5 years ago

This is for the Resolution Scaling module you are working on? I would call it something like scaling.lua.

If it literally is only encompassing all Regions/Locations then maybe directory.lua might be a good one?

29988122 commented 5 years ago

https://www.ptt.cc/bbs/FATE_GO/M.1545670814.A.C2F.html

At the major transportation hub @ TW

Cool AF

29988122 commented 5 years ago

Latest interview of Kinoko Nasu

Jpn: https://www.4gamer.net/games/266/G026651/20181228023/

Chn: https://www.ptt.cc/bbs/TypeMoon/M.1546086975.A.9B4.html


Key sentence: Kinoko: We really wanna make a clear end upon 2nd chapter finished - that is - end the current iteration of FGO, and then make a fresh start

LULWUT

Personal opinion: I'm OK with that, I guess it will only be a mid-level revamp, rather than a whole remake.