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
Implement new components for song select wedge redesign #28063
base: master
Are you sure you want to change the base?
Conversation
Tests are mostly the same as `TestSceneAdvancedStats`.
I wanna stop reading already at this point honestly. Do you realise what reviewing of a 1.7k line diffstat feels like? Do you realise the bikeshed fiasco you're opening yourself up here for by getting this entire code blob stunlocked on minute design changes? I dunno, this may be "less work" for you, but it is more work for me / other reviewers, and I see there being a 95% chance of this getting stuck in review hell again. This is especially important for me because I would like to pay very close attention to how individual components are structured to avoid similar fiascos that plague the current song select and make implementing stuff like correct beatmap invalidation basically infeasible. Please take a look at how I replaced the mod overlay for this. It was not a giant 5k line blob. |
Maybe I'm biased by wanting to make quick forward progress, but even with this many lines diff, if it's mostly drawable code and hardly any modified lines (all new lines) I'm willing to take it on – at least an initial pass to see how things read and feel. Will loop back after doing a pass. |
Sure if you want to try and stomach it I guess. But I will want to have a look too before merge even if you end up being okay with it. An important code design constraint is that every drawable component in here must support the beatmap changing under it for the invalidation flow. Preferably with a way to force a refresh manually too. If that is not the case, then I'm gonna have a problem with accepting this. |
@Joehuu what is the significance of the diff in the OP? It doesn't look to cleanly apply. |
@arflyte how much of this change is going to be thrown away in the "next" iteration? |
Seemed to be broken. Have fixed. If this does get stuck in review hell, I structured the commits here well enough for cherry-picking for individual components PRs. Skimming at the diff again, I did leave trivial code quality issues not caught by CI and some unnecessary cached fields in a test, but will fix them if the initial evaluation of this approach is tolerable. |
Layout feels really weird at non-widescreen resolutions. The left half gets too much space: Spacing is a bit weird here: Having this display jump all over the place in string length makes it very hard to visually parse: Also all the stats are just mind boggling weird layout-wise. But I guess that's something for us to fix as the designs are made without knowledge of how the information is actually used by the average player. |
Waiting for @arflyte response to #28063 (comment) before working on this again. As for the concerns, first one is from a non-pushed commit (wasn't meant to be a talking point, probably when it's actually replaced), second probably just needs a horizontal divider (like those vertical ones on figma), and third i don't really know as it's right aligned (can remove "mostly". there's this discussion, but the right align is probably a good reason). Last concern is I was just copying the layout we had (at least for advanced stats). Can probably revert back to two columns and have the left area not take up more space). |
I don't think we should be waiting on flyte responses here, especially considering that this isn't immediately going to be live on any interface. I'd just push all your remaining changes so we can fast-track this to an initial merged state. |
"Reset dependencies" already sets the beatmap to null.
@Joehuu is this ready for a full code review and/or should it be taken out of draft? |
This is ready for review, yes. Forgot to take this out of draft. |
Where's the individual component PRs?
Considered PRing individual components (and replacing old ones) more effort because:
My original proposal for the process was just implementing the component and not replacing the old one, but also may be harder to review as it'll just be a single component (content) in a test with no context.
The goal of this PR is preparation for just replacing the old wedge. It does the bare minimum: no skinning (e.g. collapsing), no new density graph, old details tab w/ metadata and online stuff is still a thing (newest design iteration may not have it displayed by default). Also had beatmap info overlay / online in mind when I did this long before, so there are
APIBeatmap
cases with some testing.Code follows the
Content
terminology like results screen components when the component contains text and has no background. The LOC is probably daunting, but If you don't count the tests / header and usings, it is probably under 1000 with some reusing of old components' code.The interim design is following the very first figma iteration for the difficulty name / length and bpm content, and for object count / difficulty settings content, it follows the current layout but has three columns because the left side expanded. The header is not implemented as I believe that takes too much height room (it's still in multi though). Also ignores the latest figma design linked above that removes bars entirely.
The old-to-new wedge replacement diff is below. Didn't apply it for now to ease LOC.
There are some TODOs, and I wrote this after midnight, so opening as draft for general feedback for now:
TODOs (some todos in code not listed):