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

Add Unity.Mathematics support #1690

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

KonH
Copy link
Contributor

@KonH KonH commented Oct 13, 2023

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

@KonH KonH changed the title Add support to Unity..Mathematics.int2 Add support to Unity.Mathematics.int2 Oct 13, 2023
@neuecc
Copy link
Member

neuecc commented Oct 16, 2023

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

@KonH
Copy link
Contributor Author

KonH commented Oct 17, 2023

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.

@neuecc
Copy link
Member

neuecc commented Oct 18, 2023

@KonH
Copy link
Contributor Author

KonH commented Oct 18, 2023

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

@AArnott AArnott added the unity label Oct 19, 2023
@KonH
Copy link
Contributor Author

KonH commented Oct 19, 2023

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

@KonH KonH changed the title Add support to Unity.Mathematics.int2 Add Unity.Mathematics support Oct 19, 2023
@KonH
Copy link
Contributor Author

KonH commented Oct 19, 2023

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

@neuecc
Copy link
Member

neuecc commented Oct 20, 2023

thanks, I'll check soon.

@KonH
Copy link
Contributor Author

KonH commented Oct 24, 2023

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

@KonH
Copy link
Contributor Author

KonH commented Oct 27, 2023

Any updates?

@neuecc
Copy link
Member

neuecc commented Oct 30, 2023

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.
Cysharp/UniTask#98
Therefore, could you please separate the assemblies?
Like MessagePack.UnityMathmatics.

@KonH
Copy link
Contributor Author

KonH commented Oct 31, 2023

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?

@AArnott
Copy link
Collaborator

AArnott commented Jan 22, 2024

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

@neuecc
Copy link
Member

neuecc commented Jan 24, 2024

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

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.

FormatterNotRegisteredException: Unity.Mathematics.int2 is not registered in resolver
3 participants