[Meta] Design changes - Githubissues

Discussion source: https://discord.gg/2tt5Der

@waicool20 and I were discussing a variety of development goals and ideas, and I feel that they should be cataloged in a place that @mrmin123 can review and comment on, as I know the discord chat can fly by while away.

We should have a discussion about optimization and porting over certain functions into a more efficient framework (i.e. Kotlin), as well as the developer workflow. Ideally a develop/optimization branch to commit/merge features/enhancements into would be nice, and from there we can test and integrate before merging into master.
As we reach some sense of code maturity, there are a lot of fixes and enhancements to be done on the older modules, such as the quest and repair module. While they work just fine as they are, there is always room for improvement, and such improvements will be documented in their own issue.
This goes along with the above, but it may be worth having a discussion about involving developers like @waicool20 or even myself more closer in the design process, by perhaps allowing access to a develop/optimization branch and pull requests without approval. I know @mrmin123 can be busy at times just like the rest of us, but formalizing a dev team structure might be beneficial. We all want a better scripting tool to allow us to be lazy. \o/

For further/detailed context, visit the discord and jump to the start of the general channel. That's the start of our winding discussion.

HI @R-Jimenez and @waicool20, some interesting discussion here.

I'll preface this by saying that after the event and merging of your two PRs (which still require changes and/or checking), I was planning on a complete refactor of the codebase due to its combined maturity, (spaghetti) complexity, and near (as far as I'm concerned) feature-completeness.

To address your points:

Your discussion on Discord indicated frustration with the speed of some aspects of the code (submarine switch caching, repairs, quests), but performance gains in these do not even require a complete refactoring, and can be addressed with minor tweaks to the existing code. The reason I haven't done this is due to 1. lack of time and 2. me waiting on kancolle-auto being more feature-complete so I had a better idea of how to refactor for the future. I've already met the requirements for point 2, but am waiting on point 1 (hopefully in the next few weeks).

Porting over to an entirely different language (kotlin, in this case) has no other advantage other than it being in a language that I guess both of you are more familiar with. Performance and even multithreading is a moot point since the Python code is ultimately compiled via Jython. And yes, Sikuli Python does support multithreading, I just never used it because I didn't see the need for it at the time.

On a related note, given your familiarity with Java, and unfamiliarity (or in @waicool20's case, a distaste for) for Python means that I do not feel comfortable giving non-approved merge ability into master. And that doesn't even touch upon the fact that I still have a short-term vision for kancolle-auto that I don't know the two of you share. Do note that this isn't a comment on your coding abilities (I'm sure your coding skills are superb), but more of an observation and feeling I had while reviewing PRs and thinking about where I want to take this project in the future.

And for the state of the codebase and featureset right now, please understand that this is a passion hobby-project for me. I took a ~200-line script that could only do expeditions and made into what it is now, slowly over the course of almost 2 years. Some features were fleshed out when I wasn't as familiar with Sikuli's capabilities, some were haphazardly added on to pre-existing features, and others needed quick patches because Kantai Collection got updated. It's not pretty, but the project got to this point due to a long series of events, not just me one day deciding to code this thing in one way and being done with it. Given that, I find the whole 'implemented the basic thing and be done with it' comment somewhat rude... If it truly was the most basic thing it wouldn't have so many features and be so robust in the ability to configure it and use it. It's always easier to find fault in the work of others, and I'm not going to sit here and say that kancolle-auto is perfect, but I'd appreciate it if you didn't actively disparage my efforts to date.

Apologies if I sound defensive in all of this, but I've poured a lot of time and thought into this project and it's still something I want to shepherd it into a state I haven't reached yet. The userbase, for better or for worse, has grown beyond what I would have ever expected, but at the end of the day it's still my personal project. Until my feelings on that changes, I will take the community's contributions/comments/criticisms into account, but I will ultimately have the final say as the maintainer of the project. If you find this unacceptable, go ahead and fork away. I can't stop you there, as that's just the nature of OSS development.

First off, being defensive is expected. This is your baby after all, and I think any developer can identify with that. I apologize if our discussion and this post came off as confrontational, as that was not the intention at all, nor to try and pressure you into action.

My intent was simply to let you know that we are happy to help with ideas and effort, and want to offer our perspective and experiences to the project. I know personally that this script has given me a passion for KC beyond what would naturally have come, as well as been a fun hobby project outside of work (which is something I find hard to do). Anyways, discussing your comments in order:

Certainly. Perhaps it's the code freeze giving me/us time to think on new features without too much pressure, but again this (at least personally) isn't a critique of the code, but just things that could be better. I enjoy the challenge of enhancing code without much disturbance to the rest of the system (my day job is integration of legacy systems into a mediator), so I think you and I are on the same page, and I'm sure @waicool20 can come around if given some more room to build out more sufficient modules.
I honestly forgot about Jython until half-way through our discussion, so it's kind of a moot point for me. I also have never written a single line of kotlin code, but alternatives are interesting to glance at.
I'm a java aficionado, but my previous work was script testing in python for network switches, so I certainly have no unfamiliarity (maybe some rust was shaken off, can't confirm). And of course, I'm not asking for write permissions for the whole repo. I'm honestly not sure if github has the ability to allow branch-specific write access like a private git server, so perhaps my original plan is not feasible.
And it's definitely something to be proud of. I know I didn't say it, but I'm sorry our discussion ended up insulting you, and I'm sure @waicool20 will apologize too. nudge nudge

I'm glad we could have this little discussion, as it has cleared up a bit about how we can go forward. I myself am more than happy to go along with your vision and help with what I can, and hope you find my ideas and vision inline with that, or at least willing to discuss a reasonable alternative. It's the least I can do for the use I've gotten out of this passion project of yours. I also hope @waicool20 chimes in with his thoughts, as then it will be a proper conversation.

Haha, I didn't mean that as coming off as a rude statement, sorry if that offended you in anyway. It was more meant to mean that some parts of the code base were an obvious?/looked rush, some parts of the code after digging through it, has many loose ends, stuff that was left forgotten? or maybe plain monkey-patched to death that could be tied up. Well I just assumed either you forgot or you were waiting for a time like this for a great refactor and didn't want to bother writing code that would just get rewritten soon. It really is a great work and I appreciate it ;) I'd be writing my own from scratch than contributing to the codebase if it weren't the case Once again I apologize.

And I'm fine without write access... that's what forks are for, I quite like the single merger model to be begin with since it ensures that all code it's consistent. Though that doesn't stop us from a having a reviewer model where a PR is only accepted after certain people (the reviewers) agree. Final say is on @mrmin123 So meanwhile I'll continue to play within the realms of my own fork.

As for the python/java/kotlin thing i do think its better to continue the script with python. First, I'd reckon that not as many people know or even have heard kotlin, so.....not really that great also for collaboration. It's also 1 reason why I'm keeping my sub switch module to myself without opening a PR, its more of a proof of concept that show cases the potential optimizations the current script can take and probably finally integrated into kc-auto through a full rewrite to python, though I guess it's worth taking reference in which points can be improved when refactoring the codebase. And on the side note, I generally don't like dynamic languages in general, Python otherwise is a superb with its wide range of libraries and community. Javascript on the other hand is one of the worst case offenders amongst the modern languages since it's both dynamic AND weakly typed. Well hope this doesn't turn into a "This language is better" war.

Furthur expanding on my kotlin module, those are only improvements that can be done "locally", as in the optimizations only affect that module. I'm thinking that many "global" optimizations can come from

Caching of the game state, in a single singleton/static object, stuff like ship data should be kept inside.
Better global timing eg. my custom has function that has() a short timeout of only 0.5s vs the current vanilla sikuli function exists() which defaults at 2s, which adds up a lot over the course of several cycles. So defining a standard set of wrapper utility functions might prove to be a useful thing so we can tweak these values.
While it indeed is a PITA, defining hard coordinates in some cases where appropriate can speed up things significantly. As you probably know, Sikuli searches are much faster in the case of a smaller region.

And while I'm at it I'll throw in some more ideas if we are going through with this refactor since it'll probably be much easier and less spaghetti if we had some forethought about it. I also propose leveraging using the standard input and allowing configuration changes on the fly. Of course it only takes effect after the current cycle is finished. It can also be used to signal the script to pause during certain breakpoints, eg. during sleep time or in between nodes etc. Given the rise in popularity in using the Kaga/kancolle-auto combo, I think this will be a fantastic feature in terms of integration. I believe the config reader could benefit from using some python/java? libraries to marshall and demarshall a defined settings object, and then instead running that settings object through a separate checker function.

Cheers

Thanks for chiming in and clarifying! I apologize for any misunderstanding from my end.

I also hope that the planned refactor will allay many of the performance issues with the script. I do plan on incorporating many of the suggestions mentioned here and elsewhere (position-based node selections on sorties, separated out repair module, more efficient search parameters, etc). I also plan on splitting some of the monolithic methods into smaller, more maintainable ones. Modules will also get an overhaul so they can more accurately convey information to and fro, as opposed to just returning a True or False for success or failure. The eventual hope being able to make other independent modules to be more cognizant of each others' statuses.

I will also say that I've made events code freezes other than for event assets and critical event-related bugfixes because I've been burned by releasing new features during events in the past. One slightly under-tested feature breaking the script during an event is no fun for anybody.

I'm going to make a separate ticket outlining the planned changes for the refactor but I'll leave this ticket open for further discussion.

Cool that we cleared the misunderstanding.

On the position based node selections: I propose that we can leverage Sikulis find function to track the coordinates of the ship icon during a sortie, and based on the x-y coords of the returned region, infer what node the fleet went to, so end result is that we can define a Map onto what specific sortie actions we can take.

eg. in Json format since I'm not sure how this could be presented in INI format

{ 
  "Nodes": [
    {
      "D" : {
        "Formation" : "Line Abreast",
        "NightBattle" : false
      }
    },
    {
      "F" : {
        "Formation" : "Line Abreast",
        "NightBattle" : false
      }
    },
    {
      "K" : {
        "Formation" : "Line Abreast",
        "NightBattle" : false
      }
    },
    {
      "J" : {
        "Formation" : "Line Ahead",
        "NightBattle" : false
      }
    }
  ]
}

Above example for more flexibility on sortieing based on 4-3 leveling, kancolle-auto as it is now I compromise between F and J node by going diamond formation. Optimally it would be Line Ahead for J and Line Abreast for F.

Having a separate branch with untested features would be a nice addition I think and would make testing more accessible since GitHub can just zip the contents. And chill and relax, getting burned coding something just produces garbage code anyways.

mrmin123 / kancolle-auto

[Meta] Design changes #369