Tournaments

[blackshadowshade]

This pull request is going to be a full implementation of single elimination tournaments, an initial attempt at addressing #462.

To do

• Responder tests
• RandomAI compatibility (code will be submitted in a separate pull request)
• Clearing the current UI with testers
  • Reduce number of tables to three: Closed, Open, Active
  • Move open tournaments that you have joined to the bottom of the Open table
  • Display tournament number (+ verb View or Join + colour?) in the leftmost column
  • For open tournament pages that you are not part of, change wording to Join Tournament
  • For open tournament pages that you are a part of, make it easy to change your button
  • For active tournaments, put games in chronological order
  • irilyth and devious to review whether issues raised in Tournaments #2131 (comment) have been addressed
  • Consider workflow when initially visiting a tournament page
  • Fix error message when joining a tournament without selecting a button
  • Check absence of open / close triangles on Tournaments page
  • Add number of rounds to tournament page
  • Consider how to deal with game messages generated when a tournament changes rounds or finishes
  • Make tournament winner more prominent
• Opening new issues regarding usability and evolution of the UI
  • Have some functionality to view a list of completed tournaments (e.g., Tournament History page)
• QUnit tests for the most important (possibly immutable) parts of the UI
• CircleCI
• Add tournament API methods to python API client (copy code in Tournaments #2131 (comment))
• Add additional responderTest helper functions to test/src/api/responderTestFramework.php (diff in Tournaments #2131 (comment))
• Code cleanliness issues raised by cgolubi1 in testing, which should be either addressed or responded to:
  • Why is "Newgame" now a global in jshintrc?
  • Rename the BMInterfaceTournament function update_tournament(), because that function does not implement the updateTournament API call, so it's confusing for them to have the same name.
• Bugs found via cgolubi1's API testing:
  • Backend allows creation of tournaments with any number of players, should only allow 4, 8, 16, 32 like the UI does
  • Backend should set a message if the player tries to create a tournament with an unreasonable integer number of wins, such as 0 or 6
  • Backend should set a message on successful tournament creation
  • All four of the API methods updateTournament, dismissTournament, followTournament, and unfollowTournament should call their tournament ID argument by the same argument name (either tournament or tournamentId, whichever you prefer)
  • dismissTournament does not work at all due to a code bug (it's using a BMDB function with the wrong arguments)
  • createTournament fails with an internal error when called with an invalid tournamentType argument
  • Missing argument to validate_join_tournament() on line 624 of BMInterfaceTournament.php
  • Spurious isTest check in interface update_tournament() function makes responder testing impossible
  • BMTournament contains a direct call to shuffle() --- in the BM codebase, we don't use randomization directly, but rather use the bm_rand() function to generate random numbers in a testing-safe way.

Dev site: https://462-tournament.blackshadowshade.dev.buttonweavers.com/

[blackshadowshade]

If anyone was interested, I've just updated this pull request invisibly (via a rebase). It now has unit tests up to and including creation of the first round of a single elimination tournament.

[cgolubi1]

What's up with this pull request? It's never been commented on by anyone except blackshadowshade, and the last comment on it was in November 2016. Shadowshade, can you weigh in a bit on what you think the current state is and what we're looking for? It's a big diff, and at least some things in the game logic have changed since it was created. As far as you're concerned, what's the next step for this PR?

[blackshadowshade]

Currently, this isn't passing unit tests, but that's not surprising. I'll probably be able to fix that quite easily. Most of the code is entirely new classes, so they don't impinge at all on the current codebase.

I'd like comments on how I've designed the generic tournament structure, and especially on how specific tournaments are specified. Have I put in enough options to allow us to cater to special tournaments in the future?

I added the concrete example of single elimination tournaments so people could better envisage exactly how to specify the rules that determine the behaviour of a tournament.

Also, if anyone has any comments on the basic structure for specifying specific buttons for a game/tournament, I'd be interested in hearing them.

[blackshadowshade]

Alternatively, if no one is especially interested in the details of the implementation, I'd like to know what I need to do to get this pull request accepted, considering that it's currently not supposed to affect any current behaviour.

[irilyth]

I'm interested in looking at this and commenting at some point, although I'm more interested in the high-level stuff like what kinds of tournaments are supported and how do they work, than in the implementation details of whether game objects have info about what tournament the game is a part of or tournament objects have info about what games are a part of them, or whatever.

Is this at a point where a forum thread on the site would be useful? Cycozar was asking about tournaments on an unrelated thread recently. Or are you more looking for feedback on the low-level basics, which don't really affect the end-user experience, in which case a bunch of end users may not be what you want yet?

[blackshadowshade]

I guess the point here is that the high-level stuff is intimately associated with the low-level implementation.

For example, I've written the tournament logic so that we can vary many things, including the logic determining who has 'won' each game, how to work out matchups in each round, how to know when to advance from one round to the other, and so on.

I'm looking for any sort of feedback, but I guess if you want guidance, maybe have a look at what range of tournament types you would like to run, and whether you think the current code even comes close to accommodating such a tournament.

[irilyth]

Ah, makes sense.

maybe have a look at what range of tournament types you would like to run, and whether you think the current code even comes close to accommodating such a tournament.

Will this be obvious from the code, if I'm not real good at SQL and PHP? If we post a link to the PR on a forum thread about tournament design, would other non-programmers who are interested in running tournaments be able to give feedback?

[blackshadowshade]

(Note that the BMTournamentSingleElimination code is almost but not entirely complete, since I don't specify the bit where we determine $tournament->remainingPlayerIdArray.)

[blackshadowshade]

Maybe start from the file BMTournamentSingleElimination.php, which is supposed to contain the entire specification of a tournament type. You'll notice it only fleshes out two particular functions: how to set up games each round, and how to know when a tournament has completed.

[blackshadowshade]

And sure, I'd be happy to have many eyes look at the code structure, with the caveat that it's not entirely complete, and that it has no UI component at the moment.

[blackshadowshade]

I've added the logic to advance a tournament when a round is complete.

[blackshadowshade]

Hmm. I think it would be easier to just have a separate branch that i submit and we merge afterwards (rather than you trying to fold that branch into this PR) --- that way if there are changes that need to be made near the end, we don't have to both deal with those. Are you okay with that?

I'm okay with that, just means you have to do a little more shuffling with your workspace, but if it doesn't bother you, that's fine.

[cgolubi1]

i'm doing some standard replay testing against the tournament site, to make sure nothing obvious broke with the base game logic.

And i ran some automated tournament testing, and that seemed fine.

Here's the easy choice for the random_ai stuff. Why don't you just apply this diff to random_ai.py in this branch:

diff --git a/tools/api-client/python/lib/random_ai.py b/tools/api-client/python/lib/random_ai.py
index 14121288..7f247456 100755
--- a/tools/api-client/python/lib/random_ai.py
+++ b/tools/api-client/python/lib/random_ai.py
@@ -151,6 +151,8 @@ UNUSED_DURING_AUTOPLAY_KEYS = [
   'gameId',
   'maxWins',
   'previousGameId',
+  'tournamentId',
+  'tournamentRoundNumber',
 
   # keys within playerDataArray
   'playerColor',

I think that's all we need for normal replay to continue to work, and then i can commit the tournament replay scripts at my leisure without a lot of workspace futzing.

[dwvanstone]

I'm very happy with all of the new changes! There are still some surprises but I feel like this is so much closer. I am just testing cancel today.

• When a tournament is cancelled it shows up in the "Closed Tournaments" tab (for the creator and anyone who is following the tournament). I was surprised by that ... but it seems consistent with how we handle game creation. This seems okay.

• When you withdraw a game (when you've picked out the opponent and are waiting for them to accept) you get a popup that says "Are you SURE you want to withdraw this game?" I suggest we have a similar popup message that warns people when they hit Cancel Tournament.

• I create a tournament and then cancel it. On the tournament page it correctly says that the tournament is cancelled, but it asks me if I want to "Unfollow tournament". I don't think it should allow me to follow or unfollow a tournament after it is cancelled. (If anything it should ask me if I want to Dismiss the tournament.)

A similar thing happens to a player who followed the tournament [but didn't join] before it was cancelled.

• Player A creates a tournament. Player B joins the tournament with a button and remains on that page. Player A cancels the tournament. Player B clicks either Leave Tournament or Change Button (and selects a button). They get the message below, even though the tournament never started.

I'll test more tomorrow.

[cgolubi1]

One quick thing i wanted to follow up on: we [mostly James and i, but reported on this thread] decided that from an API/player-visible standpoint, we'd use the "unfollow" verb for people who are not joined to a tournament (including the creator, who does not automatically join), and the "dismiss" verb for people who are joined to the tournament. (See the state diagram above, which is also under notes/logic_diagrams in the code branch.)

My recommendation is that we stick with that for now, since it sounds like what you're seeing is consistent with that behavior (a creator and a follower were both showed "unfollow" language), and we can always change/simplify it in the future if we don't like it.

[cgolubi1]

Good catch about the misleading "The tournament has already started" wording - maybe we just flip the message to say something more like "The tournament is no longer in pre-play stage, and memberships can no longer be changed", or, y'know, some more elegant wording that means that.

[dwvanstone]

• I created a 4-player, 1-round, single elimination tournament. When I try to join the tournament it says the tournament has 2 rounds. Similarly, when I create a 4-player, 5-round tournament the tournament page says it is 2 rounds.

Ah - I figured out the # of rounds is based on the number of players, i.e., the number of rounds of the tournament not the number of rounds per game in the tournament. I think having the # of rounds/game is useful data when choosing to join a tournament, although the creator could put that information in the game description.

Everything else I tested looks great! I really like having the forum link to the tournaments, I love being able to follow/unfollow a tournament from the tournament overview page, and I think the flow works well. Thank you for all of the hard work.

[blackshadowshade]

Makes sense, we should definitely have "rounds per game" and "number of tournament rounds" as separate things both listed in the summary.

[cgolubi1]

I had a replay site playing a bunch of replayed and novel conventional (nontournament) games to look for problems, and didn't find any. So that's fine.

[blackshadowshade]

I've made those changes, plus fixed the text when specifying the end condition for a tournament game from "Each game has xxx rounds" to "Each game is played until one player has won: xxx rounds".

[cgolubi1]

I rebuilt the dev site at https://462-tournament.blackshadowshade.dev.buttonweavers.com/ui/ with shadowshade's latest changes--- please take a look, testers!

[dwvanstone]

*I made a tournament where each game is only one round. The text put an "s" after round:

"Each game is played until one player has won: 1 rounds"     

• I made a tournament and hit Join Tournament. I select my button and hit "Join Tournament". This warning pops up:

  "Are you SURE you want to cancel this tournament?"

It should not put up this warning when I'm joining the tournament.

The same thing happens if I wasn't the player that created the tournament.

• Similarly ... if I hit "Leave Tournament" after joining the tournament the same warning pops up. It also happens if I use "Change button" to change the button I'm playing in the tournament.

• It does work as I would expect when I hit Cancel Tournament.

• The cancel text asks if we want to cancel the tournament. The options are "Cancel" and "OK". Will people think that hitting "Cancel" will cancel the tournament? I think it's okay as is but I thought I'd mention it.

[blackshadowshade]

I've fixed the logic around the confirm button. I must have been really tired when I wrote that code! (I'm really tired now, so it's not unlikely.)

I've also fixed the erroneous 's'.

Regarding the buttons, I could work around this by using a JQuery UI custom dialog widget if people thought it would be a good idea (link for self: https://jqueryui.com/dialog/#modal-confirmation). I'm not sure it's worth the effort though.

[cgolubi1]

Regarding the buttons, I could work around this by using a JQuery UI custom dialog widget if people thought it would be a good idea (link for self: https://jqueryui.com/dialog/#modal-confirmation). I'm not sure it's worth the effort though.

Are you talking about the "cancel" text here? Why not just wordsmith the text on the confirmation message, to say something like "Are you sure you want to cancel the tournament (press OK to proceed with cancellation)?" It's not going to be used very often, so it's okay if it's a little kludgy IMO.

[dwvanstone]

In my opinion you can leave the cancel text alone - I think I was more amused than anything. I doubt anyone will actually be confused by the interface.

[cgolubi1]

I updated the dev site at https://462-tournament.blackshadowshade.dev.buttonweavers.com/ui/ with shadowshade's latest fixes.

[dwvanstone]

It all looks good! Some notes for the future:

• We have the concept of "Tournament Legal" buttons and "Tournament"s. These terms might be confusing to new players since they are not connected.

• The Getting Started page could probably be reviewed will be out of date once this goes live. (For instance, how to join a game now that there are tournaments.) This shouldn't block the release in my opinion.

• After I lose a game and am out of a tournament I might want to unfollow the tournament even though I'm a participant. (Especially if it's a long tournament.) For the future.

[blackshadowshade]

So, @cgolubi1, where are we with this pull request? What more do we need to get it across the line?

And did you want me to rebase to one commit yet?

[cgolubi1]

I think we're ready to rebase down to one commit.

You still have a couple of things unchecked on the checklist, and i think we talked about them, but want to summarize in a comment here and either check them off, or modify the text in the checklist to say we decided they weren't blockers?

[blackshadowshade]

I've rebased to one commit and tidied up the commit message a little.

I've checked off the boxes for "Responder tests" and "Clearing the UI with testers".

We have some QUnit tests, but there are definitely many empty ones. I don't consider this to be a blocker. I'm happy to consider this to be technical debt for the moment. We will definitely add these when we have real or potential issues with the UI, or when we require the UI to be locked down for some reason.

[cgolubi1]

Sure. Let's do this. I feel like we should have a ribbon-cutting ceremony or something instead of just clicking merge.

Here goes, i guess?

\ /
 X
O O