opened
vininew921
username_0
Fixes #12823

Moved the applause sound object from the AccuracyCircle to the ResultScreen. Timed the delay so it matches the old one.
Fixed applause sound stopping after switching scores
issue
created
bdach
username_1
@username_0 it is not required to close pull requests after checks fail. you can reopen and push to the same branch you PRed this from (on your fork) to fix them.
comment
created
peppy
username_2
Which is fine, but all numbers should be drawn from constants, and match with `master`.
comment
created
bdach
username_1
I'm not sure I follow. The adjusted value I suggested in my review comment *is* the same value, temporally. The number changes because the delay starts in a different component, with a different lifetime start, so delays are naturally affected. And the original constant is one that comes out of thin air.

I can make videos to show that it's the same, but at the end of the day, from the context I have the best that can be done is `-1440 + resize_duration + top_layer_expand_delay`. I'm not sure how to derive `-1440` originally.

Am I missing some context again?
comment
created
peppy
username_2
I want to be able to review this by only reading the code. each portion of the duration which needs to be offset should be from a shared constant so it's clear why the value is what it is. And the original `1440` should be visible.
comment
created
bdach
username_1
Sure, okay. So I guess I factor out `-1440` to a public constant on `AccuracyCircle`, expose the other two ones I need from `ScorePanel`, and combine those in `ResultsScreen`? Is that right?

Although even then it wouldn't be immediately visible why `resize_duration + top_layer_expand_delay` is a thing from the diff. I'd probably need to expose a sum of those and use that in both places where it needs to go (one at existing site in `ScorePanel`, one in `ResultsScreen`).
comment
created
peppy
username_2
If this is decided as the correct direction for fixing this issue, yep, that sounds fine.

I'd hope we could come up with something more contained, but I haven't looked into it yet.
comment
created
bdach
username_1
I can't say I'm seeing an alternative as of right now, at least one that isn't a variant on `AlwaysPresent`. And it does make more sense to have an applause sound effect at screen-level to me than down in the accuracy circle.

I guess I'll just go make changes as proposed above and if you can figure out something better, it can be opened as a separate PR superseding this one.
comment
created
vininew921
username_0
Not sure why the CI check failed, I ran the same tests on my end and everything worked fine. I'm not that familiar with CI, so I don't know if it was just a fluke or a real error.
comment
created
peppy
username_2
I've added back test coverage in `TestSceneResultsScreen`. Let's go with this for now.
comment