Add role checks (like IsScp) to Role Extensions / RoleTypeId#177
Add role checks (like IsScp) to Role Extensions / RoleTypeId#177VALERA771 merged 15 commits intoExMod-Team:devfrom LumiFae:master
Conversation
|
No clue why this added 2 others, also no clue why the version got changed what |
|
There, fixed |
|
Ok, so after discussing with moddedmcplayer, they said I should add IsHuman even though it exists natively inside PlayerRoles.RoleTypeId, should I add loads of other teams too based on what Exiled.API.Features.Player has before we merge this? |
|
Could be very subjective, discussing with Exiled maintainers. |
I'd add them and then replace the ones in Player to wrap these. |
|
Wait so you'll add some of the methods or me? I don't mind making them. Just tell me what you think may be useful and I'll start |
louis1706
left a comment
louis1706
left a comment
There was a problem hiding this comment.
Remove the change that removes "," character
louis1706
left a comment
louis1706
left a comment
There was a problem hiding this comment.
I don't think this is needed "here"
|
Would be better to have it where you have the IsNtf property's |
|
Hey @louis1706 those characters being removed do nothing in actuality, just my code editor told me they should be removed so I removed them. Code still works fine. I will go ahead and move it to the IsNtf property |
|
Just had a look, there is no IsNtf property in this file. Should I move it beside |
They actually needed to prevent warning that put error in our repo because we wanted them to be put preventing useless changed line when you just add a new value under it |
Could you add it in both |
Sorry but I don't understand any of that |
Add both IsNtf and IsScp? Ok I'll try add IsChaos and stuff too |
I mean both file but that not bad too |
|
@louis1706 have a look at the changes now |
LumiFae
changed the title
|
Won't this create a problem with ambiguous reference? Like I think that if yes then some plugins will require recompiling |
|
Unfortunately, yes I think it will, but blame @moddedmcplayer for that as he was adamant that I had to add IsHuman. What would you recommend @VALERA771 |
Correct if I'm wrong but like quite certain the compiler turns extensions methods into normal calls..? |
|
I'll try to find time to test it but would be good if somebody do it faster than me (I'll be home only in 4 hours). I think we can leave this methods if they won't cause issues on plugins but imho one day we should stop replacing base game methods |
|
|
Yeah if PlayerRoles exists it makes sense for it to only use PlayerRoles.RoleTypeId.IsHuman as shown by the above screenshot. I don't think this will cause any errors honestly. But just let me know what you want me to do and I'll do it. But I just want this PR merged at some point lol |
| /// </summary> | ||
| /// <param name="roleType">The <see cref="RoleTypeId"/>.</param> | ||
| /// <returns>A boolean which is true when the role is a military role.</returns> | ||
| public static bool IsMilitary(this RoleTypeId roleType) => roleType.IsNtf() || roleType.IsChaos(); |
There was a problem hiding this comment.
currently this includes Guards because of the other thing I mentioned, I think you should specify that this doesn't (or does if it's intended) include them
There was a problem hiding this comment.
It should grab guards as they are military, but if you want the docs to be changed to mention guards are included in both foundation and military I can do that
There was a problem hiding this comment.
personally I wouldn't count guards as military, that's why I'd like you to specifically mention if they're counted or not
There was a problem hiding this comment.
Maybe I'll add smth to remove guards from military as I agree it's weird for them to be in there
There was a problem hiding this comment.
If you could specify in its docs that doesn't include guards that'd be great as well since some people might be confused
|
@louis1706 @Mikihero can you all check out these changes and let me know what you think. |
| /// </summary> | ||
| /// <param name="roleType">The <see cref="RoleTypeId"/>.</param> | ||
| /// <returns>A boolean which is true when the role is a military role. Does not include Facility Guards.</returns> | ||
| public static bool IsMilitary(this RoleTypeId roleType) => roleType.IsNtf() || roleType.IsChaos(); |
There was a problem hiding this comment.
Just does
Ismilitary(this RoleTypeId roletype) => !roletype.IsCivilian()
I know this goes against what miki said but that what basegame believe for armor consumption of stamina for example
There was a problem hiding this comment.
Idk can we count tutorial as civilian (iirc when I checked it last time RoleTypeId.Tutorial.IsCivilian() will return false)
There was a problem hiding this comment.
Tutorial is not civilian, no. so it will count as military, this change shouldn't be done @louis1706 as what I've done is perfect, this is just adding more work and more calculations for no reason.
There was a problem hiding this comment.
technically guard is a military class, I've said otherwise before because it doesn't make sense to me but that's how they're classified on the wiki, this method should reflect that
There was a problem hiding this comment.
to make it work you can just add || roletype == RoleTypeId.FacilityGuard, I don't think there really is a better solution since negating IsCivilian would return shit like tutorial etc.
There was a problem hiding this comment.
yeah, but this PR has been merged, if I could directly commit I would
|
@VALERA771 I honestly think this can just be committed, we have 4 approvals for petes sake. I don't think there will be any more good requests to come out of this. |
6 participants
Description
Add IsScp to Role Extensions to easily check if the Role is an SCP role, I noticed there was .IsHuman and .IsAlive, but no .IsScp which could be useful to some people
What is the current behavior? (You can also link to an open issue here)
You either have to do !RoleTypeId.IsHuman && RoleTypeId.IsAlive or RoleTypeId.Team == Team.SCPs to check if the team is SCPs
What is the new behavior? (if this is a feature change)
You can now check with RoleTypeId.IsScp to see if the team is an SCP team or not.
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No, nobody will need to change anything if they don't want to.
Other information:
Types of changes
Submission checklist
Patches (if there are any changes related to Harmony patches)
Other