Feature: [GS] Scriptable league tables #10001

ldpl · 2022-09-04T19:20:12Z

Default company league table came from the original game and has long lost its purpose since. As the base game has no goal it's kind of pointless to rank companies by some arbitrary rating that doesn't even facilitate good gameplay. So while adding goals to the game is the job of gamescripts, right now they have no way to change the league table mechanics having to misuse global goals for that purpose leading to player confusion.
So I designed a GS API for league tables based on the "league tables" of CityMania servers. As they're the most complex ones I've seen and cover all other use cases I know of, nicely showcasing:

There may be multiple league tables, not just one.
Score can be of some custom type like time.
Ordering can also be quite complex (companies that reached the goal are ordered by time, who didn't by population).
It can include companies that are not in the game anymore
May need to include custom information (e.g. list of players).

So I kind of got it all working but before I go finish all the boring technical details I'd like to see what opinions there are on this PR in general and some details in particular:

Naming of things. Currently, I called the table itself LeagueTable and its elements (rows) LeagueTableElement like in story pages. But it seems kinda long and generic, should I just call them rows? Also, I added Link, LinkType and LinkTargetID types that currently are kind of out of the scope of LeagueTables but are only used by them. The idea is to eventually use them in other scriptable UI like goals or story book but I don't want to do that in this PR. So should I name them like LeagueTableLink for now? I'm also looking for better names for all the other stuff.
Default league table and scoring. As I said, I don't see much point in keeping it in the game anymore. But in case someone feels strongly about it I kinda wanted to avoid removing it in this PR. So far I made it so that it shows default league menu options (league table, detailed rating, highscores) only if there are no custom league tables in the game. If there are it only shows the list of tables as GS author can implement highscores as a custom league table if they wish so and making custom rating details is just something I don't see possible without a full framework for custom UI.
Attributes. LeagueTable is quite simple and only has a title, header and footer.
LeagueTableElement though has a lot:
- table - id of the table it belongs to
- rating - hidden value used only for ordering elements in the table.
- score - string with a visible score to show.
- text - main text of the element (i.e. company name).
- link - Link(target_type, target_id) for the object to open when this element is clicked.
So I separated the logical "score" used for sorting and the visual one because, as I already said, the score can be quite complex and I don't think the game should limit it to certain types. And yet it needs some value to use for sorting the elements.
GS API. Currently, I've added the GSLeagueTable class with the following methods:
- New(title, header, footer) -> LeagueTableID - Add a new league table.
- NewElement(table, rating, company, text, score, link_type, link_target) -> LeagueTableElementID - Add an element (row) to the table.
- UpdateElementData(element, company, text, link_type, link_target).
- UpdateElementScore(element, rating, score).
- RemoveElement(element).
  I decided to split updating into two methods as IMO score is updated much more often and in different places than data is.

TODO:

Saveload
RTL
Comments

GS to test this (implements default league table in GS):
testleaguetable.zip

src/os/macosx/macos.mm

2TallTyler · 2022-09-05T16:10:15Z

I haven't reviewed the code or tested this, but I think this is an excellent path forward from the current scoring system. As broken as it is, I agree with your decision to keep it as a fallback when no custom scoring is present.

npabisz · 2022-09-09T22:36:18Z

As owner of GS server I agree that whole system of goals should be reworked, I mean its 2022. This game should put more attention into multiplayer.

ldpl · 2022-09-20T21:42:18Z

Since no one commented on the api or naming I went on with what I had and finished the PR.

James103 · 2022-10-18T15:23:52Z

Reimplementing the CityMania goal tables in the new league table system as it currently stands (with no header or footer) would mean losing the ability to have a detailed description of what the goal is or scores mean.

For example, without a header or footer, the league table would only have a title of "Goal: 10,000 people", the elements, and nothing else. With a header, it can be used to show the full goal and what the scores mean. With a footer, it can be used to show something along the lines of "... and X more" if there are too many companies/entries to show at once.

Therefore, league tables should (in this implementation) allow for a header and/or footer.

ldpl · 2022-10-27T08:10:10Z

I added header a footer. Doesn't look particularly nice but so is the rest of scriptable UI.
I've updated the test GS to showcase them.

P.S. Stupid github doesn't want to attach new screenshot for some reason :(

LordAro

No issues in principle. Mostly nitpicks

src/league_cmd.cpp

src/league_gui.cpp

LordAro · 2022-11-12T17:51:23Z

src/league_gui.cpp

+
+class PerformanceLeagueWindow : public Window {
+private:
+	GUIList<const Company*> companies;


I know this was in the original code, but why does there need to be a copy of the companies list stored within the window?

As I understand it, because it's inconvenient to keep list constantly updated, so to avoid glitches due to resorting it stores a copy and updates it episodically with full redraw of the window.

LordAro · 2022-11-12T17:53:37Z

src/league_gui.cpp

+}
+
+
+class ScriptLeagueWindow : public Window {


Could do with some doxy comments throughout

LordAro · 2022-11-12T17:56:59Z

src/league_type.h

+};
+
+typedef uint8 LeagueTableID; ///< ID of a league table
+struct LeagueTable;


Feels odd to have these forward declared in a header. Is that normal compared to other types?

I was copying story_type.hpp, seems pretty common.

LordAro · 2022-11-12T17:58:58Z

src/script/api/script_league.cpp

+
+/* static */ ScriptLeagueTable::LeagueTableID ScriptLeagueTable::New(Text *title, Text *header, Text *footer)
+{
+	CCountedPtr<Text> title_counter(title);


this feels very std::shared_ptr... but that's something for the future.

src/toolbar_gui.cpp

LordAro · 2022-11-12T18:02:15Z

src/toolbar_gui.cpp

- *
- * @param index Graph to show.
- * @return #CBF_NONE
+ * Enum for the League Toolbar's and Graph Toolbar's related buttons.


Not actually an enum. Could it be?

Possibly, but CTMN_* in the same file isn't technically a enum either but is called so in the comment.

src/league_gui.cpp

LordAro · 2022-11-21T10:16:07Z

src/toolbar_gui.cpp

@@ -2020,7 +2057,7 @@ struct MainToolbarWindow : Window {
 			case MTHK_STORY: ShowStoryBook(_local_company); break;
 			case MTHK_GOAL: ShowGoalsList(_local_company); break;
 			case MTHK_GRAPHS: ShowOperatingProfitGraph(); break;
-			case MTHK_LEAGUE: ShowCompanyLeagueTable(); break;
+			case MTHK_LEAGUE: ShowPerformanceLeagueTable(); break;  // FIXME


LordAro

LGTM. Would appreciate another set of eyes on it

src/league_cmd.h

src/league_cmd.cpp

src/league_gui.cpp

2TallTyler · 2022-11-22T18:30:10Z

src/league_gui.cpp

+			widest_width = std::max(widest_width, GetStringBoundingBox(STR_COMPANY_LEAGUE_COMPANY_NAME).width);
+		}
+
+		this->text_width = widest_width + WidgetDimensions::scaled.hsep_indent * 3; // Keep some extra spacing


Multiplying hsep_indent by a magic number to create some new non-standard amount of space isn't particularly pretty. Is there another constant that could be used, or created?

It does happen in other places: https://github.com/OpenTTD/OpenTTD/blob/master/src/cheat_gui.cpp#L244
Idk any better way to do it.

Copy & paste of my changes too, it's basically "make some arbitrary space", which may or may not be equal in different places.

src/league_gui.cpp

ldpl force-pushed the gs-league-table branch from 9680708 to c7a4052 Compare September 4, 2022 22:15

James103 reviewed Sep 4, 2022

View reviewed changes

src/os/macosx/macos.mm Outdated Show resolved Hide resolved

ldpl force-pushed the gs-league-table branch from c7a4052 to 1493eb5 Compare September 4, 2022 22:39

ldpl force-pushed the gs-league-table branch from 1493eb5 to b6ca514 Compare September 20, 2022 21:41

ldpl marked this pull request as ready for review September 20, 2022 21:41

ldpl force-pushed the gs-league-table branch from b6ca514 to e7b6843 Compare September 20, 2022 21:43

ldpl force-pushed the gs-league-table branch from e7b6843 to ad1a7de Compare October 27, 2022 07:56

LordAro added this to the 13.0 milestone Oct 27, 2022

glx22 mentioned this pull request Nov 6, 2022

Codechange: [MinGW] use pe-bigobj-x86-64 format for x64 debug builds #10142

Merged

ldpl force-pushed the gs-league-table branch 4 times, most recently from 9809cb8 to ce2a814 Compare November 11, 2022 05:49

2TallTyler added size: large This Pull Request is large in size; review will take a while needs review: Script API Review requested from GS/AI expert labels Nov 11, 2022

LordAro reviewed Nov 12, 2022

View reviewed changes

ldpl force-pushed the gs-league-table branch from ce2a814 to 18331b2 Compare November 13, 2022 19:12

PeterN reviewed Nov 13, 2022

View reviewed changes

src/league_gui.cpp Outdated Show resolved Hide resolved

PeterN reviewed Nov 13, 2022

View reviewed changes

src/league_gui.cpp Outdated Show resolved Hide resolved

ldpl force-pushed the gs-league-table branch 2 times, most recently from 3aee5f5 to add83a4 Compare November 14, 2022 05:16

LordAro reviewed Nov 21, 2022

View reviewed changes

ldpl force-pushed the gs-league-table branch 3 times, most recently from 69ac2aa to 9097688 Compare November 21, 2022 13:50

LordAro previously approved these changes Nov 22, 2022

View reviewed changes

2TallTyler reviewed Nov 22, 2022

View reviewed changes

ldpl dismissed LordAro’s stale review via e2c18f4 November 22, 2022 20:47

ldpl force-pushed the gs-league-table branch from 9097688 to e2c18f4 Compare November 22, 2022 20:47

Feature: [GS] Scriptable league tables

30da057

ldpl force-pushed the gs-league-table branch from e2c18f4 to 30da057 Compare November 23, 2022 21:55

LordAro approved these changes Nov 24, 2022

View reviewed changes

michicc merged commit 5e14a20 into OpenTTD:master Nov 26, 2022

JGRennison mentioned this pull request Dec 3, 2022

[Bug]: Scriptable league table minor issues #10214

Closed

This was referenced Jul 2, 2023

[Inconsistency]: No 'remove table' function for GSLeagueTable, unlike GSStoryPage and GSGoal #11097

Closed

[Inconsistency]: No function to update the title, header and footer of a league table #11098

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature: [GS] Scriptable league tables #10001

Feature: [GS] Scriptable league tables #10001

ldpl commented Sep 4, 2022 •

edited

2TallTyler commented Sep 5, 2022

npabisz commented Sep 9, 2022

ldpl commented Sep 20, 2022

James103 commented Oct 18, 2022

ldpl commented Oct 27, 2022

LordAro left a comment

LordAro Nov 12, 2022

ldpl Nov 13, 2022

LordAro Nov 12, 2022

ldpl Nov 14, 2022

LordAro Nov 12, 2022

ldpl Nov 13, 2022

LordAro Nov 12, 2022

LordAro Nov 12, 2022

ldpl Nov 13, 2022

LordAro Nov 21, 2022

LordAro left a comment

2TallTyler Nov 22, 2022

ldpl Nov 22, 2022

PeterN Nov 22, 2022

Feature: [GS] Scriptable league tables #10001

Feature: [GS] Scriptable league tables #10001

Conversation

ldpl commented Sep 4, 2022 • edited

2TallTyler commented Sep 5, 2022

npabisz commented Sep 9, 2022

ldpl commented Sep 20, 2022

James103 commented Oct 18, 2022

ldpl commented Oct 27, 2022

LordAro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LordAro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ldpl commented Sep 4, 2022 •

edited