-
Notifications
You must be signed in to change notification settings - Fork 104
fix: group(rank) and renaming of some properties #676
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
Changes from 1 commit
c72eabf
cfa302d
7e673b2
a356911
3c4f0b1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -564,7 +564,12 @@ public Quaternion Rotation | |
| public PlayerPermissions RemoteAdminPermissions | ||
| { | ||
| get => (PlayerPermissions)ReferenceHub.serverRoles.Permissions; | ||
| set => ReferenceHub.serverRoles.Permissions = (ulong)value; | ||
| set | ||
| { | ||
| ReferenceHub.serverRoles.Permissions = (ulong)value; | ||
| Group.Permissions = (ulong)value; | ||
| ReferenceHub.serverRoles.FinalizeSetGroup(); | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -1033,12 +1038,11 @@ public float Stamina | |
| public bool IsStaffBypassEnabled => ReferenceHub.authManager.BypassBansFlagSet; | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the player's group name. | ||
| /// Gets the player's group name. | ||
| /// </summary> | ||
| public string GroupName | ||
| { | ||
| get => ServerStatic.PermissionsHandler.Members.TryGetValue(UserId, out string groupName) ? groupName : null; | ||
| set => ServerStatic.PermissionsHandler.Members[UserId] = value; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why removing this line ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There's no point in assigning anything to this dictionary. No functionality is called.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the setter doesn’t work then the getter won’t work. Is that not obvious? Also it’s a breaking change
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The setter and getter functions work. This only makes sense if you want to retrieve the player's group name. Setting a value in this dictionary doesn't make sense because functionally nothing will change. The player's group will remain the same. This violates data integrity. You assign a group name to a player, but in fact, they don't have the properties of that group. This is literally the most obvious idea, immediately obvious. |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
@@ -1082,18 +1086,18 @@ public UserGroup Group | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the player's rank color. | ||
| /// Gets or sets the player's badge color. | ||
| /// </summary> | ||
| public string RankColor | ||
| public string BadgeColor | ||
louis1706 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| get => ReferenceHub.serverRoles.Network_myColor; | ||
| set => ReferenceHub.serverRoles.SetColor(value); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the player's rank name. | ||
| /// Gets or sets the player's badge name. | ||
| /// </summary> | ||
| public string RankName | ||
| public string BadgeText | ||
louis1706 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| { | ||
| get => ReferenceHub.serverRoles.Network_myText; | ||
| set => ReferenceHub.serverRoles.SetText(value); | ||
|
|
@@ -1915,25 +1919,19 @@ public bool TryGetItem(ushort serial, out Item item) | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Sets the player's rank. | ||
| /// Receives an existing rank(group) or, if it doesn't exist, creates a new one and assigns it to this player. | ||
| /// </summary> | ||
| /// <param name="name">The rank name to be set.</param> | ||
| /// <param name="group">The group to be set.</param> | ||
| public void SetRank(string name, UserGroup group) | ||
| public void SetGroup(string name, UserGroup group) | ||
|
||
| { | ||
| if (ServerStatic.PermissionsHandler.Groups.TryGetValue(name, out UserGroup userGroup)) | ||
| { | ||
| userGroup.BadgeColor = group.BadgeColor; | ||
| userGroup.BadgeText = name; | ||
| userGroup.HiddenByDefault = !group.Cover; | ||
| userGroup.Cover = group.Cover; | ||
|
|
||
| ReferenceHub.serverRoles.SetGroup(userGroup, false, false); | ||
| } | ||
| else | ||
| { | ||
| ServerStatic.PermissionsHandler.Groups.Add(name, group); | ||
|
|
||
| ReferenceHub.serverRoles.SetGroup(group, false, false); | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would change the whole group permission not only the permissions for this player
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this does override the permissions for the entire group. But only if the player was assigned a group via "Player.SetRank." If we assign them a group via the "Player.Group" property, this problem doesn't occur. I spent three hours trying to reproduce this issue using the "Player.Group" property and various other manipulations, but I couldn't. However, the fact is: if you assign any permissions to a player using the current implementation of the "Player.RemoteAdminPermissions" property, they don't actually receive them. Access to the RA panel and other functionality are disabled.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked my code this way after your review. And I still couldn't figure out what was going on. Why does adding a group via a property work differently than via a method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe first try not having that event be async, while Exiled does support async events, that doesn’t mean your method will work, if anything in that method does anything with mirror somehow, it’ll probably kill the server or soft-dc the client. Also it can very easily cause other errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a deep understanding of how this game works. If no one has any ideas on how to fix this, I might revert it back to its original state in the next commit.