Refactor bot architecture to have better separation of concerns

[huiwang]

I write blog post to explain the rationals behind bot architecture designed in this Kit. http://truelaurel.com/2017/07/07/Bot-Architecture-in-CodinGame-Scala-Kit/ Please share your feedbacks.

When I write the post, I digged deeper on the design. Here are some refactorings that could make concepts better separated and hopefully it's more clear.

The main trait refactored are

• GameIO
• GameBot
• GameAccumulator
• GameSimulator

I removed the old game arenna module because I don't find it well designed to play games locally.

Please note that this change can break bots developed with the old framework but the adaptation should be pretty straight-foward.

What do you think @tyrcho @Wei-1 ?

[Wei-1]

So enemy simulation will be done in GameSimulator? Or will be pulled out and let the main structure handling it? (minimax in the simulator or main structure?)

[huiwang]

Game simulator has nothing related to strategy. It applies an action to a state and updates the state. It's about the game rule.

Minimax will be something implemented in game bot but it will call game simulator during its branching operation.

[tyrcho]

@huiwang overall it's pretty clear designed and well explained ! A few remarks :

You might want to explain more precisely the role of the Context. Is it only for initial context (like map size) or does it contain information specific to a given turn ? I'd prefer the first option, and all "mutable" information to be stored in State. OK, I got the answer by looking at trait Accumulator. I still think it would make the design simpler to have only "InitialContext" and "GameState" with only GameState being enriched over time.

The trait GameIO could be split in 3, each one holding only 1 method. I'd prefer the usability this way : you can use a lambda to implement each small trait, and the type parameters would be simpler. You can still implement all 3 in the same class if the implementations are simple.

I was always a bit uncomfortable with the Bot returning a Vector[Action], especially when the rules only allow one action. I'd prefer to see simply an Action as the return type. The developper can then choose when relevant to implement Action as a case class holding several atomic actions.

I did not understand why you mention JFR in the end ... is it some extra tool you use like visualvm ? Or is it simply a dependency used by JMH ? I've also used scalameter during wondev challenge. It works in a similar way to JMH but is easier to integrate : does not make sbt mandatory, and does not crash on App trait usage. There is less documentation on it though.

Last thing, I discovered docute.js.org a while ago and I'm using it at work to publish such documentation. It's really easy to integrate (my sample) and produces nice looking SPA site.

[huiwang]

@tyrcho thanks for the review.

Regarding the io part, I put them together because they are always used together. It's like a serialization contract we always put read and write together.

Regrading the action part, I understand why you are not comfortable with the action vector because me too:) Let's try what you suggested and see if it makes things better.

As for the jfr, it's shipped together with visualvm if i understand well. I find it quite simple and powerful to use.

Now, regarding the context part and what I called game accumulator. Context is supposed to include all information not provided inside the loop. This means global and historical information. Technically, I put context as a field in the state object. Now you make think that it could be a good idea to split the context into two parts, global and historical. Can you have a look at the gameloop object and especially the fold method and tell me what you think?

Thanks a lot!

[tyrcho]

@huiwang I made a sample design removing the context from type parameters : #46 It looks a bit simpler in my opinion.

[huiwang]

It removes one concept but I'm afraid we are making explict things too implict. After reading your review this morning, I'm almost convinced that we should seprate them more.

First, they are not changing in the same rhythm. Global information never changes. Round information is updated every round.

Second, they are different in nature. In the wondev example, the accumulator is doing nothing but in the implementation used in the contest, I accumulate historical information for enemy prediction. Therefore, if we mixed everything in the game state, we would have static, historical, real-time information in the same object.

Due to the differences in rhythm and nature, we must set default values to most of the fields because they are not all known at the same time. Moreoever, the method signature tends to be counter-intuitive, for example the def readState(turn: Int, previous: State): State. It smells that it will do more that just read.

When things are different, I think it's better to treat them differently.

[tyrcho]

I think 3b057c4 would make it slightly clearer with proper semantic for readInitialState. Then you don't need those default fields in the state because all is read at the same time in turn 0.

[tyrcho]

You said in a previous comment "Technically, I put context as a field in the state object". And I did exactly the same in my implementation of FastState during the challenge ! So why not accept that it's the simplest thing to do ? To keep the context as part of the state.

[huiwang]

Mixing context into state is the best compromise I find so far. At concept level, we have both context and state made explicit. At implementation level, they are mixed together and we have only one object to pass around.

[huiwang]

@tyrcho I merged this PR and shared the architecture post on tweeter. Hopefully, there will be more feedbacks. I really like to continue our discussion on #48 and #46.