micahstairs / bga-innovation

MIT License
6 stars 2 forks source link

Maintain splay in "Moonlight Sonata" #217

Closed micahstairs closed 2 years ago

micahstairs commented 2 years ago

NOTE: Before you begin working on this, please assign this issue to yourself. If you do not do this, then there's the possibility that 2 people will end up doing the same task, which is not a good use of time.

When we implemented the card, we assumed splay wasn't maintained, but Chris says the intent is to maintain the splay. So we need to adjust the implementation.

ultimatefiend commented 2 years ago

We also need to make sure the solution doesn't "resplay" the pile which would trigger a City draw when cities are introduced.

micahstairs commented 2 years ago

The other card we implemented already "resplays", so we will need to fix that too.

I propose we silence the logging message for this "resplay" for now, and then add a TODO to make sure we are handling that case properly with Cities later. If we don't add a TODO we are most likely going to forget about this edge case.

We can silence the logs by adding a new silent_splay parameter to our splay function, which defaults to false, and pass that into the notifyForSplay function so that we can omit the log message. We still need to send the notifications though, we just don't want to print to the game log (otherwise it looks like it's a new splay).

function splay($player_id, $target_player_id, $color, $splay_direction, $force_unsplay=false, $silent_splay=false) {
micahstairs commented 2 years ago

Then again, I'm not sure my proposed solution will actually work well, because we currently get a log message saying that the pile was reduced to 1 and became unsplayed. So we need to find a way to silence that too.

So given that the above might be complicated to do, maybe we should keep the unsplay/splay messages in this case of maintaining splay, and then just make sure we do the right thing when Cities is implemented.

ultimatefiend commented 2 years ago

sure. We could just have a special command that changes the resplay in the DB without going through the normal API?

micahstairs commented 2 years ago

Something like that, yeah.

ultimatefiend commented 2 years ago

or maybe a flag signifying a "maintain splay" that would suppress city drawing events (when the time comes)? I'm guessing the city drawing event will need to be in that splay function.

micahstairs commented 2 years ago

Yeah my guess is that it's going to take a lot of work to try to omit the unsplay/resplay log messages but it should be pretty easy to skip the city drawing event when it's time to implement Cities.

ultimatefiend commented 2 years ago

What about having a "meld from bottom" routine that handles all of this? We are working around not having that feature by first revealing and then melding.

micahstairs commented 2 years ago

Oh yeah, good point! That's definitely the best approach. We will also want to use this for "Seikilos Epitaph" too.

This solution will require touching the SQL queries a bit (since they currently assume the card isn't both starting from and going to the same pile, I think it messes up the positioning), but with some work, I think we can fix this.

Thanks for the awesome suggestion! This will add a lot less complexity than trying to add the splay hacks I mentioned above.

micahstairs commented 2 years ago

So looking at the code, I think that if we make the meld from bottom an atomic transfer, then it won't get unsplayed with the way it's currently implemented. So we will get "maintain the splay" for free.

micahstairs commented 2 years ago

I think the bug might be purely on the frontend. The database state looks correct after melding from the bottom.