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

Migrate to .NET 6 & Revision 4 #3

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

Conversation

lillo42
Copy link

@lillo42 lillo42 commented May 2, 2023

Migrate to .NET 6; also, I'm following Go approach, so there are some breaking changes.

@boronine
Copy link
Member

boronine commented May 2, 2023

Hi @lillo42, this is excellent!

I just ran your code and seeing this test failure:

 MISMATCH xyzToRgb
  expected: 0.067, 0.933, 0
    actual: 0.06666666666656182, 0.9333333333333331, -1.290967333034132E-14
    deltas: 0.00033333333343818494, 0.0003333333333330746, 1.290967333034132E-14

It looks like your JSON parsing logic is doing some rounding. The expected value above seems to be the very first item in the snapshot:

    "rgb": [
      0.0666666666666666657,
      0.933333333333333348,
      0
    ],

Looking at your code you seem to be calling GetDouble() so presumably it should have more precision, but I'm not a C# guy so I'm not sure what's going on.

@lillo42
Copy link
Author

lillo42 commented May 3, 2023

.NET double precision is a bit different than Java/Go

I open a ticket on .NET Runtime repo:
dotnet/runtime#85700

@boronine
Copy link
Member

boronine commented May 3, 2023

The discrepancy in the test is more severe though, the JSON value 0.0666666666666666657 turns into 0.067 (see the 'MISMATCH` snippet above), that's something else

@lillo42
Copy link
Author

lillo42 commented May 4, 2023

@boronine I've moved my implementation from Java to Go, and it now seems to be worked; because of that it's necessary to migrate to .NET 6 (latest LTS) because of the Math.CopySign

@lillo42 lillo42 changed the title Migrate to .NET Standard & Revision 4 Migrate to .NET 6 & Revision 4 May 4, 2023
@chucker
Copy link

chucker commented May 4, 2023

For what it’s worth, if you require .NET 6, that’ll make the library not usable for me, as I still use it in a “legacy” .NET 4.7.2 project. .NET Standard 2.0 is fine.

@lillo42
Copy link
Author

lillo42 commented May 5, 2023

@chucker I've added support to .NET Standard 2.0, I've copied most simple implementations, but on .NET 6, you will have a better performance due a hardware acceleration.

@boronine what do you about the breaking changes? It's fine or should I use double[] instead of tuple?

@boronine
Copy link
Member

boronine commented May 8, 2023

@lillo42 breaking changes are okay because rev3 -> rev4 is arguably a breaking change already. The only issue with the PR is HUSLColorConverter using the old name HUSL, it should be something like HsluvColorConverter.

Once I merge the PR I'm going to try to release v2.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants