ctm / mb2-doc

Mb2, poker software
https://devctm.com
7 stars 2 forks source link

Support re-entries #1241

Closed ctm closed 8 months ago

ctm commented 9 months ago

Make it so that people can re-enter, using the new late registration code.

ctm commented 9 months ago

I did a bunch of work on this today and then updated the issue description to have check-boxes for what I've done and what remains to be done.

I don't anticipate working on this until Tuesday or Wednesday, but I should be able to hammer out the rest fairly soon thereafter, assuming no interfering distractions.

ctm commented 9 months ago

NOTE: I may want to commit the migration to master and deploy before too long just so I don't have to juggle migrations each time I switch to this branch.

ctm commented 9 months ago

I'm deploying the migration now. There's still a bunch of work to do, but I had a few table-splitting bugs to fix.

ctm commented 9 months ago

Ugh. I've been plugging away at this and thought I was close, but I had chosen to make it so that we can represent re-entries ending at a different point than late entry, so in theory it would be possible to have re-entries stop before late entry or vice-versa. However, right now both are gated by PersonalizedRunningEvent's registration_state field which is a LateRegistrationStatus and LateRegistrationStatus does not have a way of representing a separate cutoff, because I hadn't really thought about it at the time and in the back of my mind, I knew that the WSOP structures always had late registration and re-entries tied together.

So, I need to reconsider whether I want the ability to have them be separate (and I think I do, because I don't think it's that hard to support) and then do some refactoring so that our data structures model that decision. I.e., if I unify them, then I'll need to merge some of the late-registration and re-entry fields, but if I keep them separate, I'll need to rename LateRegistrationStatus to just be RegistrationStatus and have it contain the right fields to support independent late-registration and re-entry levels.

ctm commented 9 months ago

I made a lot of progress today, but a very serious bug (#1257) showed up today and I'll need to both fix the bug as well as put in more consistency checks before I can go back to working on this issue.

ctm commented 8 months ago

The white screen is because the WebSocket for the player->observer getting closed after invite_player has sent the SitAt message. That causes a LeftTable to remove the PlayerId from the relevant TableInfo

It may be possible to simply have the LeftTable handler recognize that the table session id has changed, in which case it should leave the TableInfo alone. I'll check that after (my late) lunch.

ctm commented 8 months ago

Yup!

diff --git a/mb2/src/mb2/nick_mapper.rs b/mb2/src/mb2/nick_mapper.rs
index 0ca36b48..62dde45c 100644
--- a/mb2/src/mb2/nick_mapper.rs
+++ b/mb2/src/mb2/nick_mapper.rs
@@ -2414,7 +2414,15 @@ impl Handler<LeftTable> for NickMapper {
                     }) = self.tables.get_mut(table_id)
                     {
                         let was_observer = table_observers.remove(&table_session_id);
-                        if !was_observer && ids.remove(player_id).is_some() && ids.is_empty() {
+                        // If the table_session_id doesn't match,
+                        // don't remove the player.  This could just
+                        // be an observer that has retransitioned back
+                        // to a player by re-entering.
+                        if !was_observer
+                            && ids.get(player_id) == Some(&table_session_id)
+                            && ids.remove(player_id).is_some()
+                            && ids.is_empty()
+                        {
                             self.tables.remove(table_id);
                         }
                     }
ctm commented 8 months ago

FWIW, at this point, with my light testing re-entry "works", however it currently decides that the re-entry period is over when late registration is over, which may wind up typically being the case, but that's not how I'm going to release this.

Unfortunately, enforcing the duration check that I want requires a bit of a refactor, because of a poor choice I made when I created the LateRegistrationStatus enum. I think I can do that refactor in a single day's effort without distraction, but I have no more programming time left today (Saturday the 17th).

In addition to the above problem, there might be problems where an attacker could submit a request for a re-entry and get one that shoudn't be allowed, because although I have checks at the high level so that the UI won't offer to let someone in when someone shouldn't be let in, I don't actually remember if I wrote the proper-level server checks and I delberately didn't put anything in the db-layer to check.

Fortunately, adding these server-level checks (if they're not already there) is—at least in theory—trivial.

ctm commented 8 months ago

I thought I could finish this up today, but re_entry_info is part of RunningEvent rather than TournamentState which is ugly, but I thought it was easier and gave us flexibility that we might never want to use (the ability to limit the number of times a player can re-enter a ring game), but ReEntryInfo uses a BettingRoundOrdinal rather than a TournamentRoundIndex, which requires a conversion that I don't really think we want.

It's a little too late in the day to refactor, but I think putting re_entry_info into TournamentState and then having it contain a TournamentRoundIndex makes things less complicated.

I'll sleep on it.

ctm commented 8 months ago

It took a few hours, but after a good night's sleep I did indeed do the refactorization that I was contemplating at the end of my workday yesterday.

ctm commented 8 months ago

FWIW, once I do the work that ties the late-registration and re-entry periods together, I can deploy even if there are a few loose ends. I'd like to do that today (Wednesday, December 27th), but I have many distractions.

ctm commented 8 months ago

I have written and tested the code that ties the late-registration to the re-entry period. It works most of the time, but I've seen some glitches, so I'm trying to see how to reproduce the problem. It's a PITA, and I haven't gone running yet today and I would really like to re-run the north portion of the Foothills 50k to get a decent GPX track to share as well as an accurate distance.

Ugh!

ctm commented 8 months ago

I can reproduce two problems, trivially:

  1. if a structure has a re-entry and the late-registration period is set to none, then the re-entry period is not changed
  2. switch to WSOP 2023 Event 95, change late-registration to 4, switch to one below, switch back and late-registration is none

The first is tricky, because if we can't set the number of re-entries to 0, although we can set the re-entry info to None. However, if we set it to None, it won't come back unless someoneone reloads the browser, which isn't a super horrible bug and is something we could document.

I will look into the second problem now and attempt to fix it since there's a tiny chance that as I do so I'll think of something clever to do for the first problem.

ctm commented 8 months ago

The second problem I mention in my comment above appears to be a problem with Yew. Adding a key attribute to a select element fixes it.

ctm commented 8 months ago

I've deployed the code as is. I did think of a fix for the bug I mention two comments ago where setting the late-registration period to none doesn't zero out the re-entry period. I'm going to see if my fix works, and if so, deploy with that fix before cleaning up this issue and (maybe) posting to the BARGE list.

ctm commented 8 months ago

FWIW, the fix I had in mind when I wrote my previous comment didn't work, so I created a separate issue (#1266) and then implemented a different fix that not only worked, but was more efficient.

I still have three more checkboxes to check (or to move into their own issues) before I close this issue, but since the functionality is deployed, I'll post to the BARGE list soon.

ctm commented 8 months ago

I'm closing this issue, although I did add a new issue (#1269) that overlapped with this one