ljvmiranda921 / seagull

A Python Library for Conway's Game of Life
https://pyseagull.readthedocs.io/en/latest/index.html#
MIT License
175 stars 28 forks source link

Add rps automata #42

Open whonut opened 4 years ago

whonut commented 4 years ago

Addresses #26.

Apologies if I should've asked before cracking on with this. There are basically two main additions at present:

  1. A new rule implementing the rock-paper-scissors automota. The implementation relies on storing a copy of the board for each state in a 3D binary/boolean array. The reason I did it this way rather than using a single integer array with different numbers for each state is that it seemed the obvious way to allow use of existing Lifeforms. It occurs to me as I write this that it would be fairly easily to do this using a single 2D integer array too. I look forward to your thoughts on this.

  2. A MultiStateBoard which keeps track of automata with multiple states and allows existing lifeforms to be added in a given state.

I haven't gotten animation working at present but I thought I'd open a PR before I start hacking at existing code! I think my additions are in a good enough state to have a discussion at this point.

codecov[bot] commented 4 years ago

Codecov Report

Merging #42 into master will decrease coverage by 11.02%. The diff coverage is 14.63%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #42       +/-   ##
===========================================
- Coverage   97.76%   86.73%   -11.03%     
===========================================
  Files          12       12               
  Lines         268      309       +41     
===========================================
+ Hits          262      268        +6     
- Misses          6       41       +35     
Impacted Files Coverage Δ
seagull/rules.py 73.68% <9.09%> (-26.32%) :arrow_down:
seagull/board.py 57.62% <16.66%> (-42.38%) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 8bad223...ce58213. Read the comment docs.

ljvmiranda921 commented 4 years ago

Hi @whonut , thanks for this! Let me check your code this weekend! Hope you had fun working on it!

ljvmiranda921 commented 4 years ago

Apologies if I should've asked before cracking on with this.

No worries!

The implementation relies on storing a copy of the board for each state in a 3D binary/boolean array. The reason I did it this way rather than using a single integer array with different numbers for each state is that it seemed the obvious way to allow use of existing Lifeforms.

This is actually a better solution that what I was thinking. I realized that it might be difficult to reuse the Lifeforms if I create a board that uses different integers instead of the binary one. Would you mind elaborating on a high-level how you're implementing the logic? I assume that (forgive, I haven't looked at the code yet):

Let me look at the code in a few minutes and add my thoughts!

whonut commented 4 years ago

So I've squashed some bugs so it actually works now! 😳

The major thing that needs doing now is some sort of modification of Simulator to get it working with MultiStateBoard. I'd be inclined to reimplement Board as a special case of it and rework simulation to work with MultiStateBoard, but that's just me. We could split off the 'flattening' currently done in MultiStateBoard.view and use it in animate too. Let me know what you think.

ljvmiranda921 commented 4 years ago

I'd be inclined to reimplement Board as a special case of it and rework simulation to work with MultiStateBoard, but that's just me.

Let's treat this as another Issue or PR! I'm fine keeping them separate for now. Perhaps you can create a base class, BaseBoard that both the Board and MultiStateBoard can inherit for the Simulator? (or let MultiStateBoard inherit from Board with your modifications).

Either way, I'm fine even with having a MultiStateSimulator and just putting proper error messages when one is used for the other. I try to be careful of generalizing too early :+1:

Sorry won't be able to check this until Sunday (or the next weekend, not this week)! Will be on vacation leave for a bit to recharge! Will find a timeto read through this and add my comments!

whonut commented 4 years ago

Either way, I'm fine even with having a MultiStateSimulator and just putting proper error messages when one is used for the other. I try to be careful of generalizing too early 👍

Fair enough, I'm pretty new at this!

Sorry won't be able to check this until Sunday (or the next weekend, not this week)! Will be on vacation leave for a bit to recharge! Will find a timeto read through this and add my comments!

I look forward to it!

ljvmiranda921 commented 3 years ago

Hi @whonut , any updates on this? :smile: