fayti1703 / binarymatrixplayer Goto Github PK

BinaryMatrixEngineTest was generated using an NUnit-Test-Framework template. It should be using Microsoft's Microsoft.NET.Test.Sdk instead (see CommonLib).

ActionSet::lane can take on out-of-range values (is < 0 or > 6).

This code may infinite loop:

Note that newLength never actually grows, as it keeps being reset to this.Cards.Length * 2 -- if a simple doubling of the array size doesn't result in a large enough array for the requested capacity; this code loops indefinitely.

Possible Solutions

Initialize newLength to this.Cards.Length, then change the statement in the loop to newLength *= 2;

BinaryMatrixEngineAccessor has some missing/unimplemented features that would be nice to have for debugging and testing purposes, in particular:

• A /full-state command that dumps the entire current game state to be inspected:

  BinaryMatrixPlayer/BinaryMatrixEngineAccessor/CommandExecution.cs

  Lines 95 to 99 in 325aa5b

  	#if false
  	private static void DumpFullState(GameContext game) {
  	throw new NotImplementedException();
  	}
  	#endif

• Support for more than two players:

  BinaryMatrixPlayer/BinaryMatrixEngineAccessor/CommandExecution.cs

  Line 7 in 325aa5b

  /* FIXME?: This doesn't support multiple players per side yet */

• Some form of file input to quickly restore to a game state. Luckily binmat is entirely deterministic (minus shuffles, but we use a PRNG), so seed + actions taken is enough to encode the entire game state.
• When no file input is provided, use a random seed (make sure this is stored/displayed somewhere to allow for restores)
• Display the binlog for the current turn in the output (Added from #8)

There's no check for a completed game here:

Possible solution: at the top of the method, check for this.victor != null, and if so, return this.victor.

This error is actually returned when attempting to play cards to / declare combat in the pseudo a lane.

Relates to: #18 which should prevent actually inputting invalid lanes into an ActionSet.

@Engine/Player.cs has too many symbols defined inside it:

• enum PlayerRole
• enum ActionType
• struct ActionSet
• interface CardSpecification
• struct PlayerData
• class Player
• interface PlayerActor
• interface ActionablePlayerActor (deprecated)
• struct PlayerID

At minimum, ActionType/ActionSet/CardSpecification should be moved to a separate file (this set of types is relatively isolated).

Problem

This code here:

However, the action order of players can change from turn to turn. It is not possible to reorder the player list in the current engine, making this impossible to implement.

Possible Solutions

1. Add a new game hook that allows sorting the list of active players prior to action execution
2. Change the Player/ActionSet architecture in that code to more closely resemble how the spec¹ is written.
3. Keep the current architecture, just allow sorting the player list rejected: this breaks the current PlayerID implementation.

Spec References

• § 5.3.5. Order players and determine actions
• § 5.3.6. Game loop, steps 8.c through 8.f

This comment was written before CardList::MoveAllTo was introduced and at a time that CardList::TakeFirst still had usages -- as such, this may be outdated. Running some benchmarks would still be good here, I think. (Particularly to figure out if this is even an area that needs attention!)

(calls ArrayPool::Create(int maxArrayLength, int maxArraysPerBucket))

Some simple benchmarking and tracking of average list lengths, along with free/used;reused/newly allocated stats would be good here. These parameters were arbitrarily chosen by a neural network (read: I made them up) instead of any hard data.

The binlog is not implemented. This is an important feature to provide feedback about the game state to players.

Internal game execution operations should already be ordered in such a way that the binlog need only record them as they occur. Any situation where this is not the case should be treated as a bug.

Possible implementation

Several game execution methods will require some way to record log-relevant information. A possible implementation would be an additional out argument -- e.g. combat resolution might take an out CombatLog containing the binlog information for the resolved combat.

This would then be added to and propagated through similar structures up to the player turn execution, which would append to an array in GameContext.

GameState will also record this information.

All current testing was performed locally, using BinaryMatrixEngineAccessor in a terminal session. This does not scale, nor is it particularly exhaustive.

Adding some unit tests would allow for increased confidence that the code behaves correctly, particularly after changes to the internals.

Dependencies

• #3

The current test suite has a test coverage of 45%, which is too low for proper confidence.

This makes refactoring certain patterns hard -- e.g.

CardList cards = declval();
cards.AddRange([ new Card(EIGHT, FORM) ]);

to

CardList cards = declval();
cards.Add(new Card(EIGHT, FORM));

(should have a warning on the upper code to replace with the lower one; and an intention to do the reverse for easy adding of elements)