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

Remove FrameStableClock (and redirect usages to FrameStabilityContainer) #19776

Merged
merged 6 commits into from
Aug 15, 2022

Conversation

peppy
Copy link
Member

@peppy peppy commented Aug 15, 2022

Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

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

Looks mostly fine. Scariest thing is, I'm not really sure I can tell if the clock shuffling is correct. It looks okay, but I'm not actually sure still.

@@ -1,8 +1,6 @@
// Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
// See the LICENCE file in the repository root for full licence text.

#nullable disable
Copy link
Collaborator

Choose a reason for hiding this comment

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

Enabling NRT in this file seems a bit drive-by (there seem to be no actual changes in this file related to FrameStableClock) but ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I originally had changed in this file...

Comment on lines +70 to +71
[BackgroundDependencyLoader]
private void load(IGameplayClock? gameplayClock)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are we still changing [BackgroundDependencyLoader(true)] to NRT-annotated params after the latest mono-related fiasco? Not that I imagine this particular one will cause issues, but it's a bit touch-and-go and I'm not sure where we're at regarding this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did consider that for this specific usage, but I do believe it's only a problem in tests. Can revert if preferred.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be okay. Honestly would like to see NET6 prioritised still...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants