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

Scoreboard implementation #3953

Open
wants to merge 58 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@lkolbly
Contributor

lkolbly commented Aug 23, 2017

This implements per-world scoreboards, including handling when players move between worlds. Much of the actual business logic is handled by the client, so all the server has to do is tell the clients what the teams are.

Criteria are now a combination of an eCriteria, which specifies the class, and an integer sub criteria whose meaning changes depending on the criteria class. For example, for EntityKill and EntityKilledBy the sub criteria is the entity type number.

Note that not all criteria types are implemented, as that involves placing triggers in other parts of the code, so is a battle for another day.

@bearbin

This comment has been minimized.

Member

bearbin commented Aug 25, 2017

@lkolbly How is this implementation coming along - any chance of it being merge-ready soon?

Type = "table",
},
},
Notes = "Returns all of the players this objective is tracking",

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Players, as in cPlayer instances, or playernames, or UUIDs? Array-table, or a map?

@@ -9095,11 +9095,11 @@ a_Player:OpenWindow(Window);
Params =
{
{
Name = "string",
Name = "player",

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Name or UUID or cPlayer? Use a descriptive param name: playerName, playerUuid

}

Pkt.WriteVarInt32(static_cast<UInt32>(a_Delta.size()));
for (auto name : a_Delta)

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

const auto & name


if (a_Mode == 0)
{
std::set<AString> TeamPlayers = a_Team.GetMembers();

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Use auto

@@ -131,6 +131,8 @@ class cProtocol
virtual void SendSpawnVehicle (const cEntity & a_Vehicle, char a_VehicleType, char a_VehicleSubType) = 0;
virtual void SendStatistics (const cStatManager & a_Manager) = 0;
virtual void SendTabCompletionResults (const AStringVector & a_Results) = 0;
virtual void SendTeam (const cTeam & a_Team, Byte a_Mode) = 0;

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Use the eProtocolAction enum here as well, only convert in the last moment before sending.

AStringVector cObjective::GetPlayers(void) const
{
AStringVector Players;
for (auto player : m_Scores)

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

const auto & player

@@ -288,15 +444,17 @@ cScoreboard::cScoreboard(cWorld * a_World) : m_World(a_World)

cObjective * cScoreboard::RegisterObjective(const AString & a_Name, const AString & a_DisplayName, cObjective::eType a_Type)
{
if (m_Objectives.count(a_Name) > 0)
{
return nullptr;

This comment has been minimized.

@madmaxoft

madmaxoft Aug 25, 2017

Member

Shouldn't it return the existing objective instead?

This comment has been minimized.

@lkolbly

lkolbly Aug 25, 2017

Contributor

Maybe, I don't think so though, for two reasons:

  1. If someone creates a new objective, they probably expect it to not have any scores set (so if they create it and add 1 to someone's score, they would expect the score to be 1), and
  2. If the objective is already created, it's likely that it's being used somewhere else to track something else, so there could be a conflict between the two uses of the scoreboard (imagine if two plugins both used a scoreboard with the same name, for whatever reason).
@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Aug 25, 2017

The scoreboard sources are very, very unintuitive. Even after reading them, I have no idea how the scoreboard system works or how I could use it in a plugin.

You should add doxy-comments for all the public functions in all the scoreboard.h classes. Consider even renaming those functions to make it easier to understand what the function does (all the "Player" functions - do they work on names, uuids or cPlayer instances?), I don't think any plugin is using these, so breaking the API in this case is acceptable and very much desired.

Overall, make more use of auto and when iterating over stuff, const auto &.

@lkolbly

This comment has been minimized.

Contributor

lkolbly commented Aug 25, 2017

@bearbin Functionally speaking, it's basically fully functional except for the missing protocols, though the code still needs much polish. For the reasons @madmaxoft mentions, though, I don't feel like it's particularly great code (like, I understand how it all fits together now, but it's taken a lot of learning to get here).

@madmaxoft I'll add some documentation and some examples. Some of the Player functions (the ones in cTeam) are really Entity functions, and some (the ones in cObjective) are really key/value functions. I'll see if I can't change it around.

@@ -52,11 +58,32 @@ class cObjective
otStatEntityKilledBy

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Since the enum has been renamed, the prefix should be changed, too (the only argument against this would be when it is exported to the API, but we agreed that the API can change safely)

This comment has been minimized.

@lkolbly

lkolbly Sep 9, 2017

Contributor

Changed to "cr..."

@@ -52,11 +58,32 @@ class cObjective
otStatEntityKilledBy
};

// tolua_end
struct eType

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Since this is a struct now, it shouldn't have the e prefix.


static eType StringToType(const AString & a_Name);
static AString TypeToString(eType a_Type);

This comment has been minimized.

@madmaxoft

madmaxoft Sep 1, 2017

Member

Don't export these two to Lua API directly, but rather manually convert them to the two enum values:

cObjective:StringToType("...") -- returns number, number
cObjective:TypeToString(otDummy, 0)

This comment has been minimized.

@lkolbly

lkolbly Sep 5, 2017

Contributor

RegisterObjective's API-facing signature would have to change to take either two ints or a string (or, plugins would have to manually construct an eType (now called Criteria in my local branch)). Its current use is:

Scoreboard:RegisterObjective("list", "List Scores", cObjective:StringToType("dummy"))

Same for cScoreboard::ForEachObjectiveWith.

This comment has been minimized.

@madmaxoft

madmaxoft Sep 5, 2017

Member

Yes, and it would actually still work - Lua allows functions to return multiple values and when such a function is called as the last "parameter" of another function, all of the return values are passed:

local function giveValues()
  return "hello", 7, "worlds"
end

-- These two print the same thing:
print("hello", 7, "worlds")
print(giveValues())

This comment has been minimized.

@lkolbly

lkolbly Sep 5, 2017

Contributor

Huh, that's pretty useful (as a language feature). Today I learned.

That said, I'm not entirely sure we want to do that here. If in the future we wanted to give RegisterObjective another (optional) parameter, existing plugins would break (since it's no longer the last two parameters).

It may make sense to export cObjective::Criteria (née cObjective::eType) to the lua API (maybe even as cObjectiveCriteria). Then also instead of the static methods for converting to/from strings, those could be a Criteria method and constructor, respectively:

local Objective = Scoreboard:RegisterObjective("list", "List Scores", cObjectiveCriteria("dummy"))
LOG("Has criteria ", Objective:GetCriteria():ToString())

This comment has been minimized.

@madmaxoft

madmaxoft Sep 7, 2017

Member

I'm hesitant to introduce a new API type just for wrapping two numbers representing some type. Would it be reasonable to pack it into a single number (enum even better)? Since there is a list of all possible values, it would make sense to represent it as a single enum, possibly with helper functions to break it apart into components?

This comment has been minimized.

@lkolbly

lkolbly Sep 7, 2017

Contributor

Vanilla supports 1,791 different criterion for scoreboards (22 criterion classes + all the subcriteria), explicitly enumerating them would be tricky. But, it could be done as a bitfield (16 bits + 16 bits would work, and fit it into an int). Then lua sees it as a single number. Helper functions could be exposed so lua can query the class, convert to strings, and so forth.

@madmaxoft

This comment has been minimized.

Member

madmaxoft commented Sep 1, 2017

After starting the review, I realized that this is probably using the wrong approach to the scoreboard system and teams.

We need to decide on the representation first. Do we store the individual entity scores inside the scoreboard itself, or inside the entity representation? Do we store player teams only in the scoreboard, or as links in the player objects? How do we even represent the individual player scores for each objective? What is the API to query it / update it?

@lkolbly

This comment has been minimized.

Contributor

lkolbly commented Sep 1, 2017

Do we store the individual entity scores inside the scoreboard itself, or inside the entity representation?

Entity scores can't be stored inside a cEntity, because there can exist scores for "entities" that don't exist (if I read the question correctly). Also, the score still persists even if the entity disappears, for scores that are linked to an entity.

That said, there is still the issue of how to handle the non-dummy scores. By my reckoning, three are three broad objective criteria: Those that can represent anything (dummy), those that represent something concrete that changes on "notable events" (e.g. deathCount, playerKillCount, etc.) and those that represent fluid statistics that change often (e.g. food, air)

So there is the question of how to retrieve those stats. Right now, the system is that the player calls a scoreboard method to update all deathCount scores for the given player (there could be multiple objectives with that).

Do we store player teams only in the scoreboard, or as links in the player objects?

If a player leaves a world and comes back, the scoreboard for that world needs to know what team they were on when they left (so a cPlayer instance would have to store a cTeam pointer for every world on the server).

@lkolbly

This comment has been minimized.

Contributor

lkolbly commented Sep 9, 2017

Oh, forgot to mention in the commit message: added code to increment any crStatEntityKill objectives.

@bearbin bearbin requested review from bearbin and madmaxoft Dec 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment