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

TTT: Replace most UniqueID usage with SteamID #1182

Merged
merged 2 commits into from May 26, 2016
Merged

TTT: Replace most UniqueID usage with SteamID #1182

merged 2 commits into from May 26, 2016

Conversation

bmwalters
Copy link
Contributor

@bmwalters bmwalters commented May 22, 2016

Replaces lots of UniqueID usage with SteamID, preserving all necessary functionality.

@willox
Copy link
Contributor

willox commented May 22, 2016

I can see quite a few of these changes not being compatible with certain addons

@Kefta
Copy link
Contributor

Kefta commented May 22, 2016

Fix 'em

@bmwalters
Copy link
Contributor Author

I'll find out what changes break what and reply here.

@bmwalters
Copy link
Contributor Author

bmwalters commented May 22, 2016

  • DNA Sample field killer_uid -> killer_sid change breaks nothing (on GitHub)
  • SCORE.Events uid -> sid field change breaks nothing (on GitHub) (surprisingly)
  • ttt_transfer_credits concommand data change breaks nothing (on GitHub)
  • KARMA.RememberedPlayers index change breaks nothing (on GitHub)
  • GiveEquipmentWeapon timer name change breaks nothing (on GitHub)

but the big one:

Now so far this breakage are fairly minor as I doubt any servers use those addons (with the exception of the last one, which should no longer be used anyway). However there are some extremely major addons that this change breaks:

However, these addons only rely on this field in one or two places each. Simple changes can be made to fix them. But there is one more broken addon that will definitely cause grief:

  • TTT Defibrillator.

This addon is found on many servers and all of them must use this feature to operate successfully. The two versions that are on GitHub (@willox' and another one) are both broken by this, for example.
This will probably cause problems, as there are also versions on the workshop.

I believe this is enough for me to revert that one change. Any additional thoughts?

@Kefta
Copy link
Contributor

Kefta commented May 22, 2016

Revert it, submit pull requests to all of the addons changing that one thing, warn people of deprecation. We can tell people how to fix it if they ask on Facepunch. I can help contact the authors on Steam if you wish for the workshop versions.

@robotboy655 robotboy655 added the TTT The pull request is for TTT and will be handled by svdm. label May 22, 2016
@bmwalters
Copy link
Contributor Author

Reverted.

@Bo98
Copy link
Contributor

Bo98 commented May 22, 2016

How about, for backwards compatibility, just simply defining both a field for UniqueID and Steam ID (with a note telling developers to use the Steam ID one) and using the Steam ID one throughout TTT?

rag.uqid = ply:UniqueID() -- Backwards compatibility: use the SteamID field below instead.
rag.sid = ply:SteamID()

That should get the best of both worlds.

@bmwalters
Copy link
Contributor Author

@Bo98 Good idea. Done.

rag.uqid = ply:UniqueID()
rag.sid = ply:SteamID()

rag.uqid == ply:UniqueID() -- backwards compatibility; use rag.sid instead
Copy link
Collaborator

Choose a reason for hiding this comment

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

You've got an = too many there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How embarrassing. Fixed.

@svdm svdm merged commit eaf7d98 into Facepunch:master May 26, 2016
@bmwalters bmwalters deleted the ttt-uniqueid branch May 26, 2016 17:50
@markusmarkusz
Copy link
Contributor

This killed every usage with bots and every equipment menu reskin and every custom score event. :(

@robotboy655
Copy link
Collaborator

Should've used SteamID64 tbh

@markusmarkusz
Copy link
Contributor

markusmarkusz commented Oct 22, 2016

This would also break things. SteamID64 returns no value for bots on client state.

@markusmarkusz
Copy link
Contributor

What can be used to identify players and bots on client and server?
Player:AccountID() doesn't work with bots.
Player:SteamID() has two problems: On server every bot has “BOT“ as SteamID. On client it returns no “NULL“ for bots.
Player:SteamID64() has one problem: On client it returns no value for bots.
Player:UniqueID() can cause problems.
I assume that Player:UserID() wouldn't be a good solusion.

@Kefta
Copy link
Contributor

Kefta commented Oct 24, 2016

There isn't any default solution for unique IDs that handle bots included, unfortunately. It would be fine if the SteamID function had a separate numerical universe for bots or something that would solve conflicts, but I doubt it'd ever be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TTT The pull request is for TTT and will be handled by svdm.
Projects
None yet
7 participants