-
Notifications
You must be signed in to change notification settings - Fork 53
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
fix for rivers and bridges snake start positions #85
Conversation
Codecov Report
@@ Coverage Diff @@
## main #85 +/- ##
==========================================
- Coverage 72.84% 70.86% -1.98%
==========================================
Files 22 22
Lines 1517 1579 +62
==========================================
+ Hits 1105 1119 +14
- Misses 381 428 +47
- Partials 31 32 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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 good, just have a couple of questions!
err := rules.PlaceSnakesAtPositions(tempBoardState, startPositions) | ||
if err != nil { | ||
return err | ||
} |
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.
This doesn't randomize the starting positions, right? I think we want them to be randomized.
The battlesnake
CLI seems to accidentally cause this to happen by storing the snakes in a map, which randomly orders them.
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 randomly ordering makes sense. I was expecting it to be the case, but the Python code I was translating didn't have any and I strictly followed its approach. But maybe we should change that? It would be pretty simple to add some randomness.
@@ -590,6 +640,59 @@ func (m RiverAndBridgesHazardsMap) UpdateBoard(lastBoardState *rules.BoardState, | |||
return StandardMap{}.UpdateBoard(lastBoardState, settings, editor) | |||
} | |||
|
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.
The starting places don't seem to be symmetrical - what's the thinking behind which ones are left out? I don't think we necessarily need support for 16 snakes (i.e. 4 in each quadrant) but just wondering how they've been chosen.
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.
The positions came from the Python source that Chris shared with me in the linear ticket.
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.
@robbles this was an artifact of this map originally being used for showdowns with up to 16 snakes which was covered in the original logic that Torben was porting.
I think adding some randomness is fine as long as we are trying to distribute the snakes between the quadrants relatively evenly. Ultimately I think beyond 4 snakes it is hard to do a purely "fair" distribution of snakes on this map so i think we can do whatever will make for a more interesting game
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.
Ah yeah I can see how it's important to ensure even weighting of quadrants. We couldn't naively shuffle the start points, because you could end up with some pretty unbalanced quadrants.
I think that a simple solution is to shuffle the snakes that get placed. This would add randomness and preserve the way the current start positions order keeps the quadrants evenly distributed.
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.
@chris-bsnake that's a good point about distributing evenly between quadrants. Maybe it's not worth it at this point to implement random placement - it seems complicated to actually get to something fair. I was just thinking about copying the list shuffling logic used elsewhere, but that could result in 3 snakes in 1 quadrant and the 4th in their own.
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.
@robbles what about the solution I just pushed where I shuffle the order that the snakes get placed? This way it's not entirely predictable, but we still have the quadrants distributed.
I could go further as well. This approach adds some randomness, but I could introduce more by shuffling the start points in a way that still doesn't allow one quadrant to become overloaded.
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 feel like a quadrant shuffling approach could work well
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.
Sorry, I hadn't refreshed and my comment was missing your context. I'll take a look now.
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'm not seeing a change that shuffles the order? Did that commit get pushed?
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 shuffling the snakes, then placing in order based on the start positions being evenly distributed makes sense though. That sounds like a great solution.
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.
Tested this out with all 3 supported sizes and different numbers of snakes, and it all seems to work as expected.
* main: prevent negative hazard damage from healing past max health (BattlesnakeOfficial#91) allow placement of up to 16 snakes on xlarge board (BattlesnakeOfficial#90) update example output from battlesnake play --help DEV 1404: Support streaming CLI games to the browser board (BattlesnakeOfficial#88) speeding up how fast the expanding box grows from every 15 to every 12 turns (BattlesnakeOfficial#89) fix for rivers and bridges snake start positions (BattlesnakeOfficial#85)
This PR adds fixed snake start positions for the rivers and bridges map.