Turn difficulty & performance attributes into structs #30727
Conversation
|
@minisbett can you please do some performance tests (either via existing tests or adding new ones to cover diffcalc flow if it doesn't exist) so the core team doesn't get distracted by this? We'd want to ensure this isn't losing any processing efficiency. |
Structs Classes Would've been surprised if structs are slower by any relevant amount. I've committed the tests. |
|
Imo all the performance discussions above are unnecessary. Attributes aren't what diffcalc is allocating 2+GB/sec of. If me caching hitobjects barely improves performance then this change is not going to be felt. It's going to be boxed and effectively the same as a class at the end of the day. What's more important is making sure this is sanely usable in those other projects. Finally, I had similar concerns with my diffcalc refactors - do I use structs + interfaces or figure out some jank format where everything is a struct and you can somehow provide arbitrary data. I came to the same conclusion there - allocations are not worth caring about here. |
It's a peace-of-mind thing. Maybe unnecessary for you, but necessary for me to move forward quickly. It just makes sense to place the onus of this on the PR author rather than us. |
|
All performance concerns aside, there's some general consensus required that would come with implementing this PR with osu-native in mind. The discussion for that can be followed in #difficulty-osu on the devcord. tl;dr: If we would want to care about native usage of the attributes in their implementation in the Lazer code, it'd mean that any managed fields are to be avoided, as they cause increased maintaining efforts on osu-native. Which is arguably a weird constraint put on Lazer development for a rather independent project. Specifically, a nullable field in the taiko performance attributes is an issue. Removing it is not an option becaue despite having not really a use right now, with an upcoming PP rework it will turn into a difficulty attribute. So it either requires special handling for nullable values in osu-native, or there's a replacement for it's nullability (eg. I might have found a solution for nullable value types that I still need to test properly, but even if that can be handled there still needs to be a consensus that the attributes should not contain non-(nullable-) value types. The whole purpose of this PR was not having to 1:1 copy the attributes into osu-native, just as structs. But the extra handling mentioned above would likely mean that any attributes with managed fields require some kind of wrapping. |
|
Update: I managed to easily handle nullables. Therefore, the only concern there is is do we want to constrain attributes to not contain managed objects? That includes any C# classes, any arrays and strings. I believe this contraint is sort-of given by nature, at least for difficulty attributes, as they also need to be serializable for the database. As for performance attributes, there's no contraints but on the other hand performance attributes are useless besides the total PP & the PP per skill for displaying in the result screen. But to be able to confidentally progress on osu-native, I'd like to have this security and as a part of it get this PR merged. |
Will osu-native handle it fine as-is or it will it need additional changes? Also for other reviewers, keep in mind that o-n is not an official project (yet) and highly experimental. I suggest treating this as a code quality change first. |
|
!diffcalc RULESET=osu |
Yes, osu-native will be able to handle nullables. The reason behind my trouble was that I've been creating a new project for C# bindings based on .NET 9, and therefore VS suggested to me to use the Specifically, the issue was the usage of the bool type in public struct Nullable<T>
{
public bool HasValue;
public T Value;
}With the new-ish source generated marshalling, the If interested, more can be found here: |
|
!diffcalc RULESET=taiko |
|
!diffcalc RULESET=catch |
|
!diffcalc RULESET=mania |
|
Difficulty calculation failed: https://github.com/ppy/osu/actions/runs/11948957279 |
| @@ -8,6 +8,7 @@ | |||
|
|
|||
| namespace osu.Game.Rulesets.Catch.Difficulty | |||
| { | |||
| [JsonObject(MemberSerialization.OptIn)] | |||
There was a problem hiding this comment.
what's that? why is it here?
There was a problem hiding this comment.
This attribute was previously defined on the class all attributes inherit from. Even though technically only the osu difficulty attributes need it right now, I decided to stick to how it was before.
|
I have a problem with this diff in that I don't... find it particularly compelling? Like, if I'm to take @smoogipoo's proposed premise:
this arguably does not a code quality change make. I have a distinct feeling that outside of catch's aim-attribute-is-star-rating weirdness, inheritance made sense, and there need to be concessions made in removing it (for instance, the opt-in serialisation thing which is now plastered on 4 structs, although you could say you could just remove it from places where it's irrelevant). Like at best it's a sidegrade quality-wise, and at worst it's slightly worse than master. And as stated before, there are no relevant performance implications here either. The only compelling reason I'd see to merge this is whatever the osu-native thing is trying to do, so I'm not sure how to proceed. |
bdach
force-pushed
the
diff-pp-structs
branch
from
8235746
to
d6e5a47
Compare
This turns all difficulty and performance attributes into structs, allowing them to be used via osu-native for marshalling.
The attributes should never contain anything that is not some basic numbers, but using them in osu-native highlights the necessity to keep an eye on that they always will.
Additionally, the mods were removed from the structs as they are managed objects. Because of that, the ShouldSerialize check for flashlight difficulty is handled via nullability instead.