-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Castle Wall Hazard Map #101
Conversation
Wall of hazards around the board with dangerous bridges. - add support for all standard board sizes - hazard placement for all board sizes
support 12 snakes on XLarge and XXLarge board sizes
no food in the first 10 turns
@bcambl Ooooo, this is a fun one! |
Marked as wip while reorganizing into individual map sizes. |
maps/castle_wall.go
Outdated
} | ||
|
||
// only support 8 or less snakes on boards smaller than XLarge | ||
if (len(initialBoardState.Snakes) > 8) && (initialBoardState.Width < rules.BoardSizeXLarge) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you could pass in the maximum allowed snakes as a parameter for the function and just do one check and then have each map provide a value that is associated with the map. I would try my best to keep setupCastleWallBoard relatively independent of board size, you can handle that logic in the different maps themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you can just have one check that returns ErrorTooManySnakes
maps/castle_wall.go
Outdated
} | ||
|
||
// add positions 9-12 for xlarge board size | ||
if (initialBoardState.Width >= rules.BoardSizeXLarge) && (len(initialBoardState.Snakes) > 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
much like above, could you pass in the set of starting positions to use into this function and have the CastleWallExtraLargeHazardsMap do this check and pass in the expanded set of starting positions? That way this function just has to use the start positions it is given
maps/castle_wall.go
Outdated
return nil | ||
} | ||
|
||
// max of 2 food on medium & large board |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same concept as above, pass in the max food as a parameter since it seems to be linked to map size, and just do one if statement
maps/castle_wall.go
Outdated
|
||
rand := settings.GetRand(lastBoardState.Turn) | ||
|
||
rand.Shuffle(len(castleWallMediumFood), func(i int, j int) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be misreading it, but i'm unsure why you are using the castleWallMediumFood
constant here, is it supposed to be the food
array provided as a parameter to the function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most definitely was supposed to be food
. An unfortunate side effect of a late night refactor of a refactor.
@chris-bsnake Thank you for taking time to review this PR. I have made changes based on your feedback. Please re-review and let me know if the latest commit resolves your concerns. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bcambl Thanks for making those changes!
I've got a couple of optional suggestions, but otherwise this is looking good! LMK what you think - I'm happy to merge this if you don't want to spend any more time on it 😄
break snakeLoop | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks to me like if a food overlaps with a snake body, this will still check all the existing food for collisions as well, right?
It's not a huge deal since the lists of food are relatively short and you've already made it break early when food is placed. I think this could be simplified a bit though by dropping the tileIsOccupied
var and replacing the break
statements with a continue
to the outermost (food) loop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this and you are correct. I will re-work this section.
Same issue may also exist in arcade_maze as that is where I copied from in attempt to match the project:
Line 99 in 2668788
for _, food := range foodPositions { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah - I'm not that surprised that this logic came from one of the other maps 😅
We're hoping to move a lot of this common repetitive logic into the Editor object in the near future, and we can standardize on the right way to do it at that point.
maps/castle_wall.go
Outdated
if err != nil { | ||
return err | ||
} | ||
return nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tiny style nitpick: This could just be
return setupCastleWallBoard(...)
(same for the other Setup/Update methods)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem! Funnily enough, I actually switched to the long-hand version in my previous commit for the update func to match the style of the latest rivers_and_bridges refactor.
|
||
"github.com/stretchr/testify/require" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Totally optional as these tests work just fine:
These 3 tests all seem to have the same logic, just different values for the map, sizes, and start positions. They could probably be a single table-driven test using
t.Run(m.ID(), func(t *testing.T) {
for each map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! I will re-write this.
- fixes forced food spawning infront of snake issue on large & xlarge maps with double walls
@robbles @chris-bsnake this PR should be ready for another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks again @bcambl!
Castle Wall Map
Gameplay Notes
--hazardDamagePerTurn 50
will require snakes to commit to crossing the bridges and/or collecting food.Support Matrix
Board Examples
11x11
19x19
25x25
Thoughts/Concerns/Discussions
Open to any/all suggestions or feedback from the team and community.
EDIT: Removed uninteresting map sizes and refactored PR to align with work completed in #103