Add Unity mathematics support by Hertzole · Pull Request #1312 · MessagePack-CSharp/MessagePack-CSharp · GitHub
Skip to content

Add Unity mathematics support#1312

Closed
Hertzole wants to merge 11 commits into
MessagePack-CSharp:masterfrom
Hertzole:mathematics
Closed

Add Unity mathematics support#1312
Hertzole wants to merge 11 commits into
MessagePack-CSharp:masterfrom
Hertzole:mathematics

Conversation

@Hertzole

Copy link
Copy Markdown

This commit adds support for Unity's new mathematics they introduced a while back.
Some changes had to be done to the assembly definition that would raise the minimum Unity version to 2019.2 (which is outside the LTS range anyways) because of the version defines. The mathematics will only be enabled if the package is installed.

All the formatters are inside a USE_UNITY_MATHEMATICS define.

@AArnott AArnott added the unity label Aug 19, 2021
@AArnott AArnott requested a review from neuecc August 19, 2021 19:37
@neuecc

neuecc commented Aug 20, 2021

Copy link
Copy Markdown
Member

@Hertzole

Copy link
Copy Markdown
Author

I've made a new commit with a separate assembly definition. But there are some issues I can see with this approach.

It will require users to add a new Unity mathematics resolver to their resolver collection manually since the StandardResolver can't access it. The new assembly definition needs access to the MessagePack assembly thus blocking the MessagePack assembly from referencing the Mathematics assembly definition.

@alli1999

Copy link
Copy Markdown

Hi is this issue still open to contribute in?

@neuecc

neuecc commented Aug 23, 2021

Copy link
Copy Markdown
Member

@Hertzole
thanks for quick fix!
I think that's a good design for Resolver.
Mathematics is still not a standard, and it's not that inconvenient to use (Unity often composes resolvers generated from mpc).

If the CI error can be resolved, I can merge it (what is the cause of this error......?).

@alli1999
What opinions do you have?

@Hertzole

Copy link
Copy Markdown
Author

@neuecc
I'm not quite sure why it fails. It complains about missing a define, and it does add an empty define to the list of defines. The only reason I can think of for this to happen is some Unity funkiness because this requires a new feature from 2019.2 and you're using 2019.1. Would it be possible to update it to at least 2019.4 LTS?
This is unfortunately not something I'm familiar with, or if I even can do it from my position.

@neuecc

neuecc commented Aug 23, 2021

Copy link
Copy Markdown
Member

@AArnott
Currently Unity's LTS is 2019.4, could you update CI's version?

@AArnott

AArnott commented Sep 2, 2021

Copy link
Copy Markdown
Collaborator

Oh boy. It's been so long since I set that up. I wonder how to update it.... I'll see what I can do.

@AArnott

AArnott commented Sep 5, 2021

Copy link
Copy Markdown
Collaborator

@neuecc It looks like 2019.4 is under LTS but newer versions also are. Should we just use the newest LTS version, or do you want 2019.4 specifically?

@AArnott

AArnott commented Sep 6, 2021

Copy link
Copy Markdown
Collaborator

@neuecc I have installed 2019.4 (on a new VM), but the build is failing due apparently to missing libraries. Is this because the newer Unity version has breaking changes, or because I haven't configured the VM properly?

https://dev.azure.com/ils0086/MessagePack-CSharp/_build/results?buildId=1645&view=logs&j=9016dd59-9768-5b0a-1885-184706606d87&t=7b0991c6-8b62-5a98-24fa-16dba8487d4a&l=913

-----CompilerOutput:-stdout--exitcode: 1--compilationhadfailure: True--outfile: Temp/RuntimeUnitTestToolkit.dll
Microsoft (R) Visual C# Compiler version 2.9.1.65535 (9d34608e)
Copyright (C) Microsoft Corporation. All rights reserved.

Assets/RuntimeUnitTestToolkit/Editor/UnitTestBuilder.cs(10,19): error CS0234: The type or namespace name 'EventSystems' does not exist in the namespace 'UnityEngine' (are you missing an assembly reference?)
Assets/RuntimeUnitTestToolkit/Editor/UnitTestBuilder.cs(12,19): error CS0234: The type or namespace name 'UI' does not exist in the namespace 'UnityEngine' (are you missing an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(1,7): error CS0246: The type or namespace name 'NUnit' could not be found (are you missing a using directive or an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(10,19): error CS0234: The type or namespace name 'UI' does not exist in the namespace 'UnityEngine' (are you missing an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(120,9): error CS0246: The type or namespace name 'Button' could not be found (are you missing a using directive or an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(21,16): error CS0246: The type or namespace name 'Button' could not be found (are you missing a using directive or an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(23,16): error CS0246: The type or namespace name 'Scrollbar' could not be found (are you missing a using directive or an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(25,16): error CS0246: The type or namespace name 'Text' could not be found (are you missing a using directive or an assembly reference?)
Assets/RuntimeUnitTestToolkit/UnitTestRunner.cs(26,16): error CS0246: The type or namespace name 'Scrollbar' could not be found (are you missing a using directive or an assembly reference?)

@AArnott

AArnott commented Sep 6, 2021

Copy link
Copy Markdown
Collaborator

It looks like the build break is a known breaking change in Unity, but I don't know how to translate their advice into a code change in the repo.

https://forum.unity.com/threads/error-cs0234-the-type-or-namespace-name-eventsystems-does-not-exist-in-the-namespace-unityengine.675295/

@Hertzole

Hertzole commented Sep 6, 2021

Copy link
Copy Markdown
Author

It seems like it's just missing the UI assemblies. Would it be possible to just add the UnityEngine.UI assembly reference to the RuntimeUnitTestToolkit assembly definition?

@AArnott

AArnott commented Sep 6, 2021

Copy link
Copy Markdown
Collaborator

@Hertzole Do you mean something like this? I've tried a few variations on that theme (as you can see in git history from that branch) but haven't had any success. What exactly do I need to do to get it to build?

@Hertzole

Hertzole commented Sep 6, 2021

Copy link
Copy Markdown
Author

@AArnott Yeah, that's what I had in mind. Super weird that that didn't fix it... Unfortunately, I can't test anything locally either because I couldn't get the tests to even compile when opened up in Unity. It was like nothing in MessagePack existed for them.

@AArnott

AArnott commented Sep 6, 2021

Copy link
Copy Markdown
Collaborator

If you look at our src\MessagePack.UnityClient\build.sh file, you'll see we copy some files around in the repo to get Unity to notice them. And prior to that, builds have to be run from the command line.

@Hertzole

Copy link
Copy Markdown
Author

After an unacceptably long time, I finally got around to check this out again and I got the tests to run locally without any issues.
Would it be of any worth for me to upload the updated assembly definitions that I have locally? I had to change some things with how they access the dll files because they were not being automatically referenced anymore.

@AArnott

AArnott commented Sep 28, 2021

Copy link
Copy Markdown
Collaborator

That sounds great. Yes please, @Hertzole.

@Hertzole

Copy link
Copy Markdown
Author

I've pushed the new changes to my branch here. I hope it can help!

@Hertzole

Hertzole commented Nov 1, 2021

Copy link
Copy Markdown
Author

Any update on this?

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

/azp run

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

@AArnott This is an extremely strange issue. I've pushed a new change and I hope that can improve the situation.
EDIT: Is there any caching on the library folder? Maybe it's aggressively caching something.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

Failed again. Are you testing locally with Unity 2020.3.17f1?

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

Do you think it might be something as simple as a module I haven't installed?
image

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

I realized now it wasn't including the Unity packages json, which UnityEngine.UI is a part of. If it runs now and gets all the packages, hopefully, it should work!

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

Anything else we might need? Perhaps an untracked file that's on your machine but not in git? It's still failing.

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

A wild guess but maybe it requires some project settings as well.

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

I see that it failed again. Are you sure your Unity installation contains the UnityEngine.UIModule? I don't know much about Linux but on the windows installation it should be in Editor/Data/Managed/UnityEngine/UnityEngine.UIModule

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

Yes, this file exists:

~/Unity/Hub/Editor/2020.3.17f1/Editor/Data/Managed/UnityEngine/UnityEngine.UIModule.dll

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

The interesting thing about that file is it even shows up in the log as a parameter to the compiler:

/reference:"/home/andrew/Unity/Hub/Editor/2020.3.17f1/Editor/Data/Managed/UnityEngine/UnityEngine.UIModule.dll"

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

That dll doesn't define the types that are missing in the compile errors anyway.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

It looks like Button is defined by UnityEngine.UIElementsModule:

image

Is that something you need to add a reference to as well?

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

I've added a new commit that removed the noUpm argument which stops Unity from getting the packages.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

It's taking a lot longer now. That's a good sign. Assuming it works, are there changes you've tried that can/should be reverted at this point to cleanup?

Don't worry about the failure on the mac agent. I already have a fix for that in #1345 and I plan to merge these two PRs together somehow.

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

I don't think there's anything really that needs to be cleaned up. All of the stuff would be created by Unity anyways.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

This seems to be taking too long. In prior successful unity builds, this unity package step took under a minute. But this PR is running at 16 minutes and counting. Any idea why?

@Hertzole

Hertzole commented Nov 3, 2021

Copy link
Copy Markdown
Author

Yeah, it seems a bit suspiciously long. Does the machine have an internet connection? Just to be sure Unity can actually fetch the packages.
Was there any reason that you know of that made you disable upm in the first place?
At the top of my head I can't think of a reason why it doesn't work.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

No idea why -noUpm was there. I didn't even know what it did. I probably copied it from some script that @neuecc was using earlier.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

And ya, the build agent certainly has an Internet connection. If you think it's downloading gobs of packages that could be taking a long time, I'll just let it run a while longer (I restarted it). Maybe the first successful build takes longer than subsequent ones? That would explain why I see so little CPU on the agent while it's seemingly hung.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

Well, something is very broken it seems. It's been ticking for an hour now, and CPU and network usage is almost zero. Unity still bubbles "up" to 2% CPU periodically, which is quite bizarre.

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

After the 1h timeout, this was the log:
unitypackage.log

Any ideas? Does the last line suggest a cause for a hang?

@AArnott

AArnott commented Nov 3, 2021

Copy link
Copy Markdown
Collaborator

When it works for you, are you actually running the build.sh (or build.bat) script?

@Hertzole

Hertzole commented Nov 4, 2021

Copy link
Copy Markdown
Author

I unfortunately don't have any idea what could be causing the hang.
And for when I'm testing, I just have the Unity editor open. I might be lacking tools to get the build.bat to run because it just closes a few seconds after I try to open it (after modifying it slightly so it works with my setup).

@github-actions

github-actions Bot commented Feb 8, 2022

Copy link
Copy Markdown
Contributor

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.

4 participants