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

Boxed.Mapping Unity3D support #592

Open
Razenpok opened this issue Apr 7, 2023 · 9 comments · May be fixed by #608
Open

Boxed.Mapping Unity3D support #592

Razenpok opened this issue Apr 7, 2023 · 9 comments · May be fixed by #608
Labels
enhancement Issues or pull requests adding an enhancement.

Comments

@Razenpok
Copy link

Razenpok commented Apr 7, 2023

Describe the feature

I want to use Boxed.Mapping in my Unity project, but it uses Expression.Lambda.Compile from System.Linq.Expressions here. Since compiling the game to mobile platforms requires AOT compiling, Unity forbids the usage of this method. Is it really necessary there? Is it possible to switch to plain new()?

@Razenpok Razenpok added the enhancement Issues or pull requests adding an enhancement. label Apr 7, 2023
@RehanSaeed
Copy link
Member

This is a performance enhancement. I ran the benchmark in the project with all the versions of .NET the project supports and its still faster using the factory as opposed to new().

image

Can we solve your problem by using an #if '#else` pre-processor directive to use new in this case? What is the target framework used by unity? Perhaps we can target that directly?

@Razenpok
Copy link
Author

Wow, I did not expect that these compile shenanigans beat the plain new()

The NuGet packages are available in Unity via this mirror registry. It simply downloads the .nupkg files and repacks them in a Unity-compatible format. It always takes .NETStandard2.0/.NETStandard2.1 dlls from the package, hence there is a whitelist in this repo to only provide Unity-compatible packages that don't use restricted stuff like Expression.Lambda.Compile.

Maybe you can #if #else the constructor part and publish a new Boxed.Mapping.Unity ( or Boxed.Mapping.UnityCompat or whatever) package?

@RehanSaeed
Copy link
Member

new() is pretty slow. It uses reflection under the covers.

Hmmm, I'm not familiar with Unity and was hoping it would have its own unity target framework instead of netstandard2.1. I'm not sure how I can detect this and switch to a different code path.

@Razenpok
Copy link
Author

Uhh, "new() uses reflection under the covers" part sounds super counter-intuitive. Where do I read more about it?

Regarding switching to a different code path: I don't think there's a way to detect Unity in runtime, but you can wrap the code in a #if UNITY and compile with this define symbol turned on (and publish a separate NuGet package)

#if UNITY
    public static T CreateInstance() => new T();
#else
    public static T CreateInstance() => CreateInstanceFunc();
#endif

@RehanSaeed
Copy link
Member

Oh excellent, #if UNITY was exactly what I was looking for.

I actually got help from none other than Jon Skeet:

https://stackoverflow.com/questions/46500630/how-to-improve-performance-of-c-sharp-object-mapping-code

@Razenpok
Copy link
Author

Thanks for the link!

For the record, #if UNITY is not something Unity-related, you might just as well write #if I_DONT_WANT_SPEED and it would do the same thing

@RehanSaeed
Copy link
Member

Looks like there are some defined by Unity but they're not going to help us.

https://docs.unity3d.com/560/Documentation/Manual/PlatformDependentCompilation.html

Having another NuGet package is a fair bit of extra work to support.

@Razenpok
Copy link
Author

Can I help with that? To me, it looks like a two-step task:

  1. Wrap the conflicting code in #if UNITY
  2. Duplicate Boxed.Mapping.Unity.csproj (with UNITY directive enabled) from Boxed.Mapping.csproj and put it in the same folder

From what I see, your build pipeline requires no tweaks to support new packages. And I suppose there won't be any code changes in the future that would break Unity compatibility, so the support effort should be negligible?

@RehanSaeed
Copy link
Member

Ok sure.

@Razenpok Razenpok linked a pull request May 17, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Issues or pull requests adding an enhancement.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants