Skip to content
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

Annotate ControlPoint and Mod for AOT trimming support #28503

Merged
merged 2 commits into from
Jun 17, 2024

Conversation

smoogipoo
Copy link
Contributor

@smoogipoo smoogipoo commented Jun 17, 2024

I have a native AOT compilation project here: https://github.com/smoogipoo/osu-native-diffcalc It compiles the most minimal types from osu+osu-framework+osuTK into a project that can be used to perform difficulty calculation with.

In it, I've had to add a few workarounds for these types which use Activator.CreateInstance() down various paths: https://github.com/smoogipoo/osu-native-diffcalc/blob/8fb300329425e52aa078b78728ba802bb8124345/Sources/osu.Game.Native.Desktop/Program.cs#L25-L71

This change should remove the need for these workarounds with very little code-wise overhead from the game itself. This is probably good practice in general though I'm not sure when we'll/if ever move towards trimming more aggressively in production.

Here's a test app:

using System.Diagnostics.CodeAnalysis;

new X().Clone().Print();
new Y().Clone().Print();

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
public class X
{
    public virtual void Print()
    {
        Console.WriteLine("hello from x!");
    }

    public X Clone() => (X)Activator.CreateInstance(GetType())!;
}

public class Y : X
{
    public override void Print()
    {
        Console.WriteLine("hello from y!");
    }
}

In .csproj:

<PublishAot>true</PublishAot>
<PublishTrimmed>true</PublishTrimmed>

@smoogipoo smoogipoo changed the title Annotate ControlPoint and Mod for AOT trimming support Annotate ControlPoint and Mod for AOT trimming support Jun 17, 2024
using Newtonsoft.Json;
using osu.Game.Graphics;
using osu.Game.Utils;
using osuTK.Graphics;

namespace osu.Game.Beatmaps.ControlPoints
{
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tested that this is enough, on the real thing, rather than on the example in the OP? On a quick look [DynamicallyAccessedMembers] has Inherited = false specified so I am not immediately confident that it will fully eliminate your workaround.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I admit I initially didn't, because it's kinda annoying to, which is why I did it in a separate example.

It does look to be working in practice though, with one caveat - I missed that LegacyControlPointInfo isn't a ControlPoint (naming... >_<) Will fix in next commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fyi, I think that this part of the documentation is the most relevant: https://github.com/dotnet/runtime/blob/257e76dfe64cb5b347aba7bc5a31a50c3c8cc106/src/libraries/System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/DynamicallyAccessedMembersAttribute.cs#L19-L21

I also noticed Inherited = false and questioned this behaviour, and had to verify myself. I also explored the alternative of using System.Diagnostics.CodeAnalysis.DynamicDependencyAttribute but that one does not work for inherited types.

Same deal with this class. Fully qualifying the type names because this
has `#nullable disable` and makes use of `NotNull` which is also present
in the `System.Diagnostics.CodeAnalysis` namespace and AAAAAAARGH
NAMESPACE CONFLICTS.
@bdach bdach merged commit 57688c2 into ppy:master Jun 17, 2024
9 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants