osu!standard parity for 2026q2 pp update - #77
Conversation
|
I agree with mirroring lazer's API as much as possible but using pre-existing code as a guide for how much is "too much" would probably help too. Seeing unused code intentionally added just to say "matching lazer's API" doesn't feel particularly helpful - if it was matching, then it wouldn't be unused. Same goes for the other places that are doing it to stay in-sync with the "old" skills - drift is expected, imo. I haven't done a full read through this PR yet so I'm not sure why you made those choices but the skill API alone just reads weird. |
tsunyoku
left a comment
tsunyoku
left a comment
There was a problem hiding this comment.
Initial thoughts, haven't done a proper comparison to lazer yet.
| if curr.idx > 2 | ||
| && let Some(osu_last_last_obj) = curr.previous(1, diff_objects) |
There was a problem hiding this comment.
I don't think there's much of an issue with just keeping the condition to let Some(osu_last_last_obj) = curr.previous(1, diff_objects). They achieve the same thing even if the semantics are different.
There was a problem hiding this comment.
True, although technically as written this would short circuit the call to curr.previous() but like you said it's just semantics at that point.
|
|
||
| // * Final velocity is being raised to a power because flow difficulty scales harder with | ||
| // * both high distance and time, and we want to account for that. | ||
| flow_diff = flow_diff.powf(1.45); |
There was a problem hiding this comment.
Some sort of preferred style from @MaxOhn would probably help but maybe this should be f64::powf(flow_diff, 1.45) instead. Same for any other usages if there are any, just noticed it's not consistent.
There was a problem hiding this comment.
Indeed, I prefer the full function syntax for these kinds of calls because it makes it slightly more similar to C#'s Math.Pow(...) syntax. Am not aware of a way to enforce this notation programmatically unfortunately. Clippy can disallow specific methods but not the syntax you call it with.
There was a problem hiding this comment.
I can go through and change the f64_foo.func(f64_bar) calls to be f64::func(f64_foo, f64_bar) in that case, no problem. I tried to go off of the existing style as much as I could and since they both seemed to be used in different places throughout the codebase I wasn't sure about it.
| fn process_internal<'a>( | ||
| &mut self, | ||
| curr: &Self::DifficultyObject<'a>, | ||
| objects: &Self::DifficultyObjects<'a>, | ||
| ) -> f64; | ||
|
|
||
| #[expect(dead_code, reason = "used by process_internal")] | ||
| fn strain_value_at<'a>( | ||
| &mut self, | ||
| curr: &Self::DifficultyObject<'a>, | ||
| objects: &Self::DifficultyObjects<'a>, | ||
| ) -> f64; | ||
|
|
||
| fn backfill_peaks<'a>( | ||
| &mut self, | ||
| curr: &Self::DifficultyObject<'a>, | ||
| objects: &Self::DifficultyObjects<'a>, | ||
| ); | ||
|
|
||
| fn save_current_peak(&mut self, section_length: f64); |
There was a problem hiding this comment.
Is implementing these in the trait not an option? Relying on the implementation inside the macros being noticed is kinda meh, and I almost left a comment asking where the code disappeared to.
There was a problem hiding this comment.
I tried my best to base these off of the existing skill traits where presumably the design philosophy was "if it needs to mutate stateful variables on the struct that are filled in by the macro, implement in the macro instead of the trait".
|
Regarding gradual calculation failures, depending on when you started copying changes over you may need one or both of these fixes: |
This is all pretty fair and valid and I would agree. Obviously everything in Like I mentioned to Max previously, rust isn't entirely my strong suit and when it comes to more involved things like the existing macros for the skill traits it was a bit of a learning experience to try and figure out what I was looking at haha. Clearly it was much easier before when the inheritance chain from skills was simply |
|
Just wanna say that a lot of rosu-pp's code is very much non-idiomatic Rust just so that it can achieve easier parity with osu, hope you don't take away any antipatterns from working with this codebase 😄 |
3 participants
Difficulty and performance tests are now passing (on unix, see related bullet) for standard. Here is my shortlist of things I think deserve a bit more attention / more experienced eye.
todo!with the intention of shadowing methods is probably not great and hard to follow.f32, but I still have no idea what was causing that.Outside of these points and general organization, I'm happy with the rest of it :)