MessagePack-CSharp Add Unity.Mathematics support

Fixes #1603 (will be appreciated for hacktoberfest-approved label)

Oct 13 '23 16:10 KonH

This PR would cause problems in an environment where Unity.Mathematics is not installed. And is it only int2?

Oct 16 '23 13:10 neuecc

Hmm, that's right, we need the ability to check availability somehow, it's better in a compile-time context. I added only int2 just to minimize iteration time, it's possible to add other types later when the whole integration circle is passed.

Oct 17 '23 15:10 KonH

You can use define like this? https://github.com/Cysharp/UniTask/blob/master/src/UniTask/Assets/Plugins/UniTask/Runtime/UniTask.asmdef#L12-L43

Oct 18 '23 02:10 neuecc

It looks like a solution. I will try it, thanks!

Oct 18 '23 16:10 KonH

I've added conditional package reference, and plan to think about more formatters little bit later

Oct 19 '23 16:10 KonH

I've added the most common types from that package, I think it's enough for initial support

Oct 19 '23 20:10 KonH

thanks, I'll check soon.

Oct 20 '23 10:10 neuecc

And also, could you please mark that PR with label hacktoberfest-accepted?
It's required for Hacktoberfest participation, as described here - https://hacktoberfest.com/participation/#pr-mr-details

Oct 24 '23 15:10 KonH

Any updates?

Oct 27 '23 15:10 KonH

Sorry for delayed review, I've checked. It's almostly ok. However, still exists "references": ["Unity.Mathematics"] in asmdef. This is a soft reference and is usually ignored if there is no reference, so there is no problem.

However, I recall that in the past such references have caused problems in dependency resolve. https://github.com/Cysharp/UniTask/issues/98 Therefore, could you please separate the assemblies? Like MessagePack.UnityMathmatics.

Oct 30 '23 12:10 neuecc

I've tried to do it and it looks like a vast refactoring:

Move Unity.Mathematics formatters from Assets/Scripts/MessagePack/Unity/Formatters.cs require references to IMessagePackFormatter
IMessagePackFormatter is located in Formatters namespaces -> new asmdef is required
IMessagePackFormatter requires MessagePackReader, MessagePackWriter, etc
MessagePackReader, MessagePackWriter are core classes -> more asmdefs are required

So it leads to big diff and I'm not sure it really wanted changes.
What do you think? Maybe I did not correctly understand you?

Oct 31 '23 16:10 KonH

@neuecc In light of #1734, do you have any fresh ideas for this?

Jan 22 '24 14:01 AArnott

I think it would be better to add it as a UnityMathmaticsResolver and move towards compositing it, rather than including it in UnityResolver.

Jan 24 '24 10:01 neuecc

Now it is separated resolver @neuecc is that what you expected to see, right? Sorry for the delay, I've missed notifications here

Jun 21 '24 18:06 KonH

Another question in my mind is whether this should target master or develop.

Jun 21 '24 18:06 AArnott

@neuecc, the author has a question for you above.

Jul 19 '24 22:07 AArnott

sorry for delayed response, I'll check soon.

Jul 23 '24 10:07 neuecc

UnityShims' UNITY_MATHEMATICS_SUPPORT is meaningless, so I think it's better to include everything.
I said I was against Unity.Mathematics being included in the main references, but it looks like that hasn't changed.
Let's stop putting it in the StandardResolver.

Jul 29 '24 07:07 neuecc

Thanks for your feedback!

UNITY_MATHEMATICS_SUPPORT removed from UnityShims
Considering Unity.Mathematics reference I left a comment previously - https://github.com/MessagePack-CSharp/MessagePack-CSharp/pull/1690#issuecomment-17875664727, are you sure that we need huge refactoring of that area or maybe it should be separated follow-up issue?
UnityMathematicsResolver usage removed from StandardResolver

Aug 02 '24 07:08 KonH