Skip to content
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

Store server ID in match table #613

Merged
merged 2 commits into from
Feb 7, 2021
Merged

Store server ID in match table #613

merged 2 commits into from
Feb 7, 2021

Conversation

nickdnk
Copy link
Collaborator

@nickdnk nickdnk commented Jan 3, 2021

Hello

As discussed in #584.

But there's some code here I don't understand. This PR needs some help as I'm not 100% sure this is right. Can you explain why this code has reversed get/set logic and always returns 0? That makes no sense at all to me.

public int Native_GetMatchID(Handle plugin, int numParams) {
SetNativeString(1, g_MatchID, GetNativeCell(2));
return 0;
}
public int Native_SetMatchID(Handle plugin, int numParams) {
GetNativeString(1, g_MatchID, sizeof(g_MatchID));
return 0;
}

I ask because I used the same pattern for this PR. I did it in the more conventional "return the integer"-way though, which may be wrong.

CC @RuneNyhuus

@Drarig29
Copy link
Contributor

Drarig29 commented Jan 3, 2021

Can you explain why this code has reversed get/set logic and always returns 0? That makes no sense at all to me.

That's how natives work in SourceMod. A native can either use the usual return keyword to return a single value (example here), or use functions to set and get "native" parameters (example here). In this context, return 0 means it worked well.

For example, with Native_GetMatchID(), you need to "return" a string, so you "set" the native string which will be returned with the SetNativeString() function.

And in Native_SetMatchID(), you use GetNativeString(), which gets the string parameter passed to the native and directly copies the value in the g_MatchId variable.

Copy link
Contributor

@Drarig29 Drarig29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more conventional "return the integer"-way is fine imo, this is what is done with Get5_MatchTeamToCSTeam() and Get5_IncreasePlayerStat().

misc/import_stats.sql Outdated Show resolved Hide resolved
@nickdnk
Copy link
Collaborator Author

nickdnk commented Jan 3, 2021

For anyone using the MySQL plugin; you must run this command on the get5 database when this gets merged:

ALTER TABLE get5_stats_matches ADD COLUMN server_id INT(10) UNSIGNED NOT NULL DEFAULT '0' AFTER team2_score
ALTER TABLE get5_stats_matches ADD INDEX server_id (server_id)

@nickdnk
Copy link
Collaborator Author

nickdnk commented Jan 4, 2021

Hey @Drarig29

I made a few adjustments to the SQL script while we're at it.

  1. There's no reason mapnumber cannot be tinyint unsigned. I don't see any way a match would ever have more than 255 maps.
  2. I changed steamid64 to bigint, as I noticed it comes from the server and is always in the STEAM64 format. There's no reason to store that as varchar. Note that the 21 length is an arbitrary limit and those have been removed for all integer types in MySQL 8 anyway. This might seem like a micro-optimization, but it is quite relevant when used in primary keys. Edit: I had made a comment about this which I deleted because my example was off by a factor of ~25 as I forgot we store stats per map and not per round. That does not, however, take away from the fact that a SteamID as varchar takes up 18 bytes (17 single-byte characters - the numbers in the string - plus 1 byte for length) vs. only 8 as a bigint.
  3. I added an index to the server_id column we just added in this PR as I forgot that originally.

All of the above is backwards-compatible (except of course for the original addition of the server_id column).

@Drarig29
Copy link
Contributor

Drarig29 commented Jan 4, 2021

I'm OK with 1. and 3. but maybe @splewis should take a look into 2. I'm OK with 2. too but I might forget something.

@RuneNyhuus
Copy link

So i have made the changes to the SQL database that @nickdnk posted.
What files do i need to change?
Sorry im new to this Github :)

@Drarig29
Copy link
Contributor

Drarig29 commented Jan 5, 2021

Don't do it yet, the PR is still open and more changes could be added.

@RuneNyhuus
Copy link

Don't do it yet, the PR is still open and more changes could be added.

Ok thank you for reply @Drarig29 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants