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
Coming back to this; I've been a bit torn apart on whether this change makes sense on Lazers' end. It might make more sense to have the attributes as POCOs. The whole reason for this change was to not have to maintain a 1:1 copy of the attributes as structs inside osu-native. The first idea was introducing interfaces in Lazer so that osu-native can contract them, but that comes with added OO complexity. But in the end, the consumer-side of osu-native still needs to implement them as a struct, so those structs could might as well be in some shared file between the two projects, making the trouble of this PR redundant. @smoogipoo what do you think on how to handle this? Maybe it's best to return to my initial approach of having structs in osu-native along with helper methods going from the managed attributes <-> structs. |
This is a valid path, and I suggest going with it for now. While I'm going to deprioritise this PR, I'd still like some form of it to arrive at some point. I think there is refactoring validity in attributes not storing |
I can agree with these two elements being a net positive, but not with this PR as a whole. If those parts can be split out of this I think they'd be merged. |
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.