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 System.Half support for .NET 5.0 #1108

Merged
merged 7 commits into from Nov 19, 2020
Merged

Add System.Half support for .NET 5.0 #1108

merged 7 commits into from Nov 19, 2020

Conversation

neuecc
Copy link
Member

@neuecc neuecc commented Nov 11, 2020

Add HalfFormatter to BuiltinResolver.

  • msgpack spec does not support float16 so use float32 instead
  • Added targetframework net5.0 and use #if NET5_0, NET5_0 must enable all NETCOREAPP2_1 optimization. Simplify #if netcoreapp filters #1107
  • Need to setup CI

@neuecc neuecc requested a review from AArnott November 11, 2020 07:04
@AArnott
Copy link
Collaborator

AArnott commented Nov 11, 2020

NET5_0 must enable all NETCOREAPP2_1 optimization

Agreed. But sadly, I just tested it and net5.0 target no longer defines NETCOREAPP as a preprocessor symbol. :( I'll research what's the best way to handle this and get back to you.

@AArnott
Copy link
Collaborator

AArnott commented Nov 11, 2020

Oh never mind. NETCOREAPP is defined for net5.0. Here's the spec BTW.

@AArnott
Copy link
Collaborator

AArnott commented Nov 11, 2020

@neuecc I believe this PR should be changed to target develop instead of master since it adds public API.

@AArnott
Copy link
Collaborator

AArnott commented Nov 11, 2020

@neuecc this PR is just a little too new for Hosted agents on Azure Pipelines, which have not yet been upgraded to VS 16.9. It only has 16.8 which does not support .NET 5.0.
We can sit tight for a week or so until Microsoft updates the agents.
Alternatively, if you don't mind removing the VSIX project that includes the analyzers, we can switch from msbuild.exe to dotnet build on Azure Pipelines, in which case we can make .NET 5.0 work immediately. VSIX-deployed analyzers are something I think we can cut support for. Many people prefer the nuget deployed analyzers anyway. Thoughts?

@neuecc
Copy link
Member Author

neuecc commented Nov 12, 2020

thank you for check it.
ok to change to develop.

VSIX-deployed analyzers are something I think we can cut support for. Many people prefer the nuget deployed analyzers anyway. Thoughts?

Yes, I wish I could, but vsix is an easy alternative because it requires a hack to get Analyzer to work in Unity csproj.
but I think it can be removed.

@crocuis
Copy link

crocuis commented Nov 12, 2020

Since the half value is ushort, it is better to serialize the value as it is without converting it to a float.

@AArnott
Copy link
Collaborator

AArnott commented Nov 12, 2020

@crocuis While your suggested change results in a smaller msgpack stream, it lies about the type to do it. A message pack reader that is simply reading values as they are typed and displaying an object tree would then totally misrepresent the value.
IMO we should keep with @neuecc's original plan until the msgpack encoding spec itself is updated to account for a 16-bit floating point number, and we can switch to that.

@AArnott AArnott mentioned this pull request Nov 12, 2020
@AArnott AArnott added this to the v2.3 milestone Nov 13, 2020
@AArnott AArnott changed the base branch from master to develop November 13, 2020 21:46
For this to work, since we now expose public API uniquely per framework, I had to split the PublicAPI files per framework as well.
@AArnott AArnott merged commit 7efe772 into develop Nov 19, 2020
@AArnott AArnott deleted the half branch November 19, 2020 15:57
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.

None yet

3 participants