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

Add client/server models & operations for "daily challenge" feature #28194

Merged
merged 1 commit into from
May 22, 2024

Conversation

bdach
Copy link
Collaborator

@bdach bdach commented May 17, 2024

Continuation of #28136.

Split out of the main MVP pull since this is the minimal set of changes required to implement necessary client-server communication.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This being a separate class may seem gratuitous, but it is actually intentional because signalr is very inflexible towards changes of method signatures once it's already used for communication, so this is supposed to be a buffer that can allow for future extension if need be.

Copy link
Contributor

@smoogipoo smoogipoo May 22, 2024

Choose a reason for hiding this comment

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

Fyi SignalR also doesn't handle changes to the serialised data gratuitously. Either way any changes to this will need some sort of update.

Edit: Or maybe it does? At least in a simple test... I'm sure I've encountered this being an issue previously with the spectator frame bundles...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to think with messagepack it should be able to handle new fields. Otherwise I'm not entirely sure why we're using messagepack (other than the size gains I guess).

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

it does handle new properties, yes. we've done this before.

Comment on lines +17 to +18
[Description("Daily Challenge")]
DailyChallenge,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As mentioned in ppy/osu-web#11204, this is required to make daily_challenge rooms deserialise at all. Without this change deserialisation just dies, which is a bit unfortunate if/once old clients ever come in contact with these - but now it is too late to do something about it anyway.

@peppy peppy merged commit 3a608aa into ppy:master May 22, 2024
11 of 17 checks passed
@bdach bdach deleted the daily-challenge-models branch May 22, 2024 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants