-
Notifications
You must be signed in to change notification settings - Fork 12
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
feat(ranking): Add RankingModes constants for GetRanking.rankingMode #79
Conversation
14ebd05
to
993e55d
Compare
Rebased onto latest and added more of the enums used by Ranking - Based on the |
I'm not sure if I should also change the types in the structs that use the enums - this feels a little invasive and likely to break downstreams, so best to use a light touch here. |
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.
We know the real names of these values now, via https://github.com/tech-ticks/RTDXTools/blob/232a7797e01369e9c12704f58fdde65dd3ac1c32/Assets/Scripts/Stubs/Generated/Assembly-CSharp/NexPlugin/Ranking.cs
Also maybe we should just add all the ranking constants while we're here?
// RankingChangeAttributeFlag is used by RankingChangeAttributesParam.ModificationFlag to set which fields should be | ||
// updated. | ||
type RankingChangeAttributeFlag uint8 |
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.
Shouldn't this just be named ModificationFlag
?
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'm not sure if we need to keep the Ranking
prefix since it's in the global namespace?
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 meant the part after the prefix. Though it being in this package is a good point. @DaniElectra what say you? If we remove the prefixes here we'd want to do it everywhere too
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.
That sounds good to me. I don't think the prefix is used anywhere else though?
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.
Just double checked, you're right I can't find anywhere that we use the prefix currently. So it should be dropped here
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'm drafting this, but am a little concerned about a type named OrderBy
(currently RankingOrderBy
) in the global namespace. The existing matchmaking ones are more uniquely named (PersistentGatheringType
) - I guess with the exception of SelectionMethod
?
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.
That shouldn't be an issue, since the import already tells that these are Ranking constants:
import (
ranking_constants "github.com/PretendoNetwork/nex-protocols-go/v2/ranking/constants"
)
ranking_constants.OrderByAscending
Also, this isn't a global namespace, the declarations are contained within the ranking/constants
directory so there wouldn't be any collisions with external names
Based on Jon's old notes and additional research
993e55d
to
de2afaa
Compare
Enums are ready for another look, happy to do a quick commit if we decide to drop the |
Except RankingMode because that one would be stupid
Added as an extra commit for revertability in case of |
In that case I think this is ready to merge |
Part of PretendoNetwork/nex-protocols-common-go#34
Changes:
Add RankingModes enum for the different GetRanking request modes. This allows downstreams like nex-protocols-common-go to implement things like Friends and Nearby leaderboards.