Conversation
|
IMO I prefer using Go constants over maps for these magic numbers, since it allows for reading the underlying value directly on IDE and is in line with making idiomatic code, which is part of our goal PretendoNetwork/nex-go#73 . It may not be the cleanest, but I think it integrates better |
This is fair 👍 in that case I propose the following standard:
Also do we want to go ahead and update the notifications types to be consistent with the rest of the protocols constants, especially now that we have the real names? It would be a breaking change but I've already started renaming certain things here anyway so this whole PR is breaking already |
|
All of that sounds good to me 👍 |
| @@ -0,0 +1,18 @@ | |||
| package constants | |||
|
|
|||
| // MatchmakeGeoIPResult represents an enum with an unknown use | |||
There was a problem hiding this comment.
I think this is probably part of the @GIR parameter of MatchmakeParam, representing the result of IP Geolocation when it has been requested by the game. I have documented some of the params here (also relevant for the ScoreBased selection method): https://gist.github.com/DaniElectra/b0f70018a71cadb329e5fa972a837ee2
There was a problem hiding this comment.
That seems likely. Good info!
@SR| Unknown | Bool | Unknown purpose, only seen in responses
Maybe this stands for ServerResponse or something? Unsure what the use is, but that name would make sense
Speaking of ScoreBased, I do have some theories about how it works but nothing concrete so I could be entirely wrong. I couldn't conclude anything definitive, but in my testing prior to the shutdown I was able to seemingly manipulate the matchamaking responses using MatchmakeParam when using the ScoreBased mode. I believe that there's some game-specific logic done on the server that assigns a "score"/"weight" to certain parameters, and the server does some sort of calculation on them, and then picks the session with the best "score"?
That would lineup pretty well with some of the information you have here too. I could could see this comparing stuff like disconnection/violation rates, countries/geo locations, etc. to find a session that best works for each specific client? Some of this data seems to be reported by the MatchmakeReferee protocol as well
Assuming that's true, maybe @TS stands for "TotalScore", as calculated by the server, which is why it's only seen in responses? Also assuming this is true, I think @SI might not refer to an attribute index like the other indexes we've see, since these would be internal values. Maybe games could have multiple "scores"/"weights" for certain values and this "index" picks which set of values to use, similar to how DataStore can have multiple configs? I could see this being useful for having different configurations for different "types" of sessions. Like maybe a game implements "normal" sessions with a higher weight on geo IP so users are more likely to match with people around them, but lowers the geo IP weight for, say, global tournaments so you have a higher chance of seeing people not around you?
Of course (assuming the above is accurate) we have no way of knowing what the calculation algorithm is, or what weights/scores are assigned for each game, or anything like that. So not a super useful mode for us I think
|
@DaniElectra I did some playing around with some ideas for notifications, how does this look? package main
import "fmt"
type NotificationEvents uint32
type subType uint32 // Exists strictly to limit the types of values that can be passed to NotificationEvents.Build
func (ne NotificationEvents) Build(subtype ...subType) NotificationEvents {
category := ne * 1000
if len(subtype) == 0 {
return category
}
return category + NotificationEvents(subtype[0])
}
const (
NotificationEventsParticipationEvent NotificationEvents = 3
)
type ParticipationEvents = subType
const (
ParticipationEventsParticipate ParticipationEvents = 1
)
func main() {
notificationTypeA := NotificationEventsParticipationEvent
notificationTypeB := NotificationEventsParticipationEvent.Build()
notificationTypeC := NotificationEventsParticipationEvent.Build(ParticipationEventsParticipate)
fmt.Println(notificationTypeA) // 3
fmt.Println(notificationTypeB) // 3000
fmt.Println(notificationTypeC) // 3001
}I think this helps clean up some of the notification handling, though I'm a bit iffy on the name |
|
On the PR you say that:
But then on the |
|
@DaniElectra I've updated the return type to just be I've also pushed the first test of setting these constants to struct fields directly. I chose |
|
I like that yeah! We could update every enum/flags parameter like this as it would integrate better in code
By this you mean updating every simple field (such as |
No I meant like going from this: func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
if _, ok := o.(AutoMatchmakeParam); !ok {
return false
}
other := o.(AutoMatchmakeParam)
if amp.StructureVersion != other.StructureVersion {
return false
}
if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
return false
}
if !amp.AdditionalParticipants.Equals(other.AdditionalParticipants) {
return false
}
if !amp.GIDForParticipationCheck.Equals(other.GIDForParticipationCheck) {
return false
}
if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
return false
}
if !amp.JoinMessage.Equals(other.JoinMessage) {
return false
}
if !amp.ParticipationCount.Equals(other.ParticipationCount) {
return false
}
if !amp.LstSearchCriteria.Equals(other.LstSearchCriteria) {
return false
}
return amp.TargetGIDs.Equals(other.TargetGIDs)
}to this: func (amp AutoMatchmakeParam) Equals(o types.RVType) bool {
if _, ok := o.(AutoMatchmakeParam); !ok {
return false
}
other := o.(AutoMatchmakeParam)
if amp.StructureVersion != other.StructureVersion {
return false
}
if !amp.SourceMatchmakeSession.Equals(other.SourceMatchmakeSession) {
return false
}
if !slices.Equal(amp.AdditionalParticipants, other.AdditionalParticipants) {
return false
}
if amp.GIDForParticipationCheck != other.GIDForParticipationCheck {
return false
}
if amp.AutoMatchmakeOption != other.AutoMatchmakeOption {
return false
}
if amp.JoinMessage != other.JoinMessage {
return false
}
if !amp.ParticipationCount != other.ParticipationCount {
return false
}
if !slices.Equal(amp.LstSearchCriteria, other.LstSearchCriteria) {
return false
}
return slices.Equal(amp.TargetGIDs, other.TargetGIDs)
}The use of the Since our simple types (UInt32, String, etc.) are all type definitions based on real, basic, types and not structs, we can make use of standard operators/methods on them in places like this |
|
@DaniElectra In |
Ohh okay, that sounds good to me!
That's because it is a string on |
I see. How do you want to handle that then? That makes it rather annoying since it would be nice to be able to reuse the existing constant here, but we'd need to handle cases where it doesn't exist. Maybe we can just add a custom value to the enum that indicates if the value is missing or not? |
|
I think it can remain as it is, allowing us to handle it as we consider on the common protocols. We have to parse the value somewhere, and I'd prefer to do it in the common protocols than adding a fictional enum value just to parse it here Also important note before we forget, we don't want to add validation checks for every field. For example, CTGP-7 modifies the |
I can opt to just remove the validation checks entirely if you think that would be better. We can handle all of that on the receiving end if need be? I've already removed validation in one place in match making (not yet pushed) because I started using a real type alias which made it impossible to add multiple |
That sounds good to me 👍 |
…th the official naming
|
Actually maybe this isn't ready for review yet? I just double checked and so far only the following protocols have had constants added to them:
That's only 9 of the 32 protocols we have implemented here currently. I'm sure there's others which could use these? Such as friends, stuff like the max number of friends you can have or the max length of a status message or something? |
…tions need to be signed
|
I have gone through every single missing protocol available here (including non-NEX) listing some constants that could be added (those with question marks mean I'm not fully certain about them since they may be impossible to know or would be unused in normal circumstances). This is a non-exhaustive list and I may have missed other constants that could be added:
|
Thank you for the list! I'm wondering how we should go about getting these values. Some of them like friends and such should be doable since that server is still online, but I'm unsure about the others? Do the clients have local checks for these maybe? Or maybe we would be just need to resort to making stuff up and fixing mistakes as we go along |
Some of them do have checks (like Messaging) but otherwise we will have to make it up |
taken mostly from wut
taken mostly from libctru
…ion to ParticipationPolicyAnybody this brings the naming convention more in line with ParticipationPolicyFriendsOnly, making the policy name match the argument name
…type int Go uses the int data type of the return value of len() and for indexing into slices, these changes prevent the need to constantly cast the values in these contexts
Obtained with the best of my ability based on context and guesses.
Resolves #XXX
Changes:
Draft for now as I want to go through and add all the missing values, as well as update any existing ones that need it.
There's a lot of work going on in the common protocols lib that is using magic numbers, such as:
This PR aims to add all the missing magic numbers/constants/enums so that this doesn't keep happening. Nearly all the data comes from https://github.com/tech-ticks/RTDXTools/, with some RE/guess work mixed in.
I would also like to discuss the possibility of moving towards maps for these enums/flags. Go doesn't have real enums, and that does make it a little awkward to use/reference them since it means we either risk name collisions for simple names (such as
Nothing) or we end up with very long-winded names (such asAutoMatchmakeOptionRecordLastGIDForParticipationCheck). So far I have opted to be inaccurate in places and ALWAYS prefix a value with its type name, even if that wasn't the case officially, so I'm not concerned about accuracy here.By moving to maps we can potentially use code like this:
Which is a bit cleaner imo.
We already treat some types this way, such as:
The only question then, if we do decide to do this, is what to do about the map/struct/type names. For example, do we forego the "real" names in places for code like this:
This works, but we'd be accessing the data as
MatchmakeOptions.None, whereMatchmakeOptionsis not technically the "real name" since that is being used up by the typeMatchmakeOptionAlso, we should discuss moving some of the struct field types to these known types now that we have them since it would make certain code a bit cleaner due to not needing to cast when comparing to constants