ppy / osu-framework

A game framework written with osu! in mind.
MIT License
1.67k stars 420 forks source link

Transforms affect new, hiding members rather than original properties #3733

Open Flutterish opened 4 years ago

Flutterish commented 4 years ago

Describe the bug: Given this code:

class Crasher : Drawable {
    new public Vector2 Position { get => base.Position; set => this.MoveTo( value ); }
}
...
AddInternal( crasher = new Crasher() );
...
crasher.MoveTo( vector, 200 );

( crasher is not used anywhere else ) osu!lazer crashes without an Exception. I assume some transformation uses reflections to set Position which results in infinite recursion.

Screenshots or videos showing encountered issue: Replicable.

osu!lazer version: 2020.717.0

Logs: network.log runtime.log updater.log database.log performance.log

smoogipoo commented 4 years ago

What you’ve mentioned is exactly what’s happening here. Don’t do that.

Flutterish commented 4 years ago

Does the position transformation not know that the target is a Drawable / implements a position interface? Metadata shows that it does. I don't think a reflection is the best way to implement this.

bdach commented 4 years ago

The transform helpers use reflection to be infinitely extensible for user code without being aware of user classes. There is no way I'm aware of to achieve that without reflection.

Flutterish commented 4 years ago

The extension methods have a generic constraint to T : Drawable. These ones can make use of that information ( as there is no other Position a drawable wants to animate ) using Transform<Vector2, Drawable>, can they not?

bdach commented 4 years ago

Whether "there is no other Position to animate" is arguable. Somebody might want to overwrite Position (as you did yourself) and have that transformed instead.

Choosing which one it should be at that point is pretty arbitrary; using the new one seems more consistent with other members unknown to Drawable to me.

Flutterish commented 4 years ago

Oh, if this is the generic use case, then absolutely. It seems to me that if one would want to do that they'd use TransformBindable or MakeTransform instead though. It might be just my ( likely less than yours ) coding experience telling me things constrained to X should only interact with X.

smoogipoo commented 4 years ago

I misread this a little bit as overriding Position, but I can see how this can get confusing and be undesirable in the context of new. May reconsider.