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

Vector2/4.Lerp do not always return value2 when amount is 1 #35529

Closed
tannergooding opened this issue Apr 27, 2020 · 14 comments · Fixed by #37764
Closed

Vector2/4.Lerp do not always return value2 when amount is 1 #35529

tannergooding opened this issue Apr 27, 2020 · 14 comments · Fixed by #37764
Assignees
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug help wanted [up-for-grabs] Good issue for external contributors

Comments

@tannergooding
Copy link
Member

Vector2.Lerp(value1, value2, amount) and Vector4.Lerp(value1, value2, amount) are currently implemented as value1 + (value2 - value1) * amount. However, due to floating-point rounding error this will not always return value2 when amount == 1.0f.

These should be updated to use the same algorithm as Vector3.Lerp(value1, value2, amount) which is (value1 * (1.0f - amount)) + (value2 * amount). This algorithm correctly accounts for the rounding error and also allows the algorithm to be freely optimized using FusedMultiplyAdd when available.

@tannergooding tannergooding added bug area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. labels Apr 27, 2020
@ghost
Copy link

ghost commented Apr 27, 2020

Tagging subscribers to this area: @tannergooding
Notify danmosemsft if you want to be subscribed.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Apr 27, 2020
@tannergooding
Copy link
Member Author

CC. @pgovind

@jeffhandley, could we get approval for this breaking change? This difference was noticed here: #35525 (comment)

@EgorBo
Copy link
Member

EgorBo commented Apr 27, 2020

Example:

public static void Main(string[] args)
{
    float v0 = 0.44728136f;
    float v1 = 0.46345946f;
    float  t = 0.26402435f;

    var lerp3 = Vector3.Lerp(new Vector3(v0), new Vector3(v1), t);
    var lerp4 = Vector4.Lerp(new Vector4(v0), new Vector4(v1), t);

    Console.WriteLine(lerp3);
    Console.WriteLine(lerp4);
}

Prints:

<0.45155275, 0.45155275, 0.45155275>
<0.45155278, 0.45155278, 0.45155278, 0.45155278>
          ^           ^           ^

@jeffhandley
Copy link
Member

@tannergooding This seems good to me, assuming this gives us comprehensive consistency. Are there any other places where the same approach would apply?

To move this forward, we should follow the model seen in a previous breaking change where we lay out the arguments for making the breaking change in 5.0 but not applying the change to down-versions. Please tag @marklio and @PriyaPurkayastha on that comment to get their approval. Their approval will suffice for getting this merged in.

@tannergooding
Copy link
Member Author

Are there any other places where the same approach would apply?

There are likely various cases where floating-point algorithms could be tweaked to provide correct results but I don't believe there is an easy way to find them. In this case, Linear Interpolation is a well known scenario and its known to be commonly impacted.

@tannergooding
Copy link
Member Author

As per the title...

Vector2.Lerp(value1, value2, amount) and Vector4.Lerp(value1, value2, amount) are currently implemented as value1 + (value2 - value1) * amount. However, due to floating-point rounding error this will not always return value2 when amount == 1.0f.
These should be updated to use the same algorithm as Vector3.Lerp(value1, value2, amount) which is (value1 * (1.0f - amount)) + (value2 * amount). This algorithm correctly accounts for the rounding error and also allows the algorithm to be freely optimized using FusedMultiplyAdd when available.

We wouldn't bring this for 3.x because:

  • It is a breaking change.
  • We don't have any customers reporting the issue

We want to fix this in 5.0 because:

  • It is a well known error case for Linear interpolation that we have already fixed for Vector3
  • It ensures all three types (Vector2, Vector3, and Vector4) have consistent behavior
  • It allows us to optimize the code on modern hardware with FusedMultiplyAdd support (a faster approach of computing the correct result is available)

CC. @marklio and @PriyaPurkayastha for approval.

If approved, I'll mark this as up-for-grabs and will pre the breaking-change doc.

@PriyaPurkayastha
Copy link

@tannergooding any idea why we have not seen customer reports on these errors? It appears that Vector*.Lerp has been around since .NET Core 1.0/.NET 4.6. You have mentioned that linear interpolation is a well known scenario impacted by this - does that mean that there was customer feedback as a result of which this has been fixed for Vector3? Vector3 fix was probably in a recent Preview and hence too early for feedback on the fix right?
Overall, I think fixing this in .NET 5 and providing breaking change documentation is the right thing to do. It appears that the goal that we are trying to achieve here is two-fold - one being accuracy i.e. ensure that the algorithm is consistent across all three Vector types and returns the correct value (which it wasn't doing earlier due to a floating-point rounding bug) and secondly, allowing optimization code on modern hardware using FusedMultiplyAdd. Accuracy is important but for some reason we haven't heard from customers about this issue and so the actual goal driving this change is that this fix also enables us to unblock code optimization mentioned earlier. The goals and change seem OK to me and we are limiting the compat impact by scoping this to .NET 5. The only thing is that I am unable to determine how prevalent the usage is and how many customers would need to update their code to work with this change.
@marklio any additional inputs/concerns regarding this change?

@tannergooding
Copy link
Member Author

Vector3 fix was probably in a recent Preview and hence too early for feedback on the fix right?

Vector3 has been different and used the "correct" algorithm back to before .NET was open sourced. I don't believe we have any documentation indicating why it differs from the other two.

You have mentioned that linear interpolation is a well known scenario impacted by this

Yes, the algorithm for correct linear interpolation which accounts for rounding error is "well known" and you can find it documented on Wikipedia, many StackOverflow posts, other math libraries, etc.
Vector3 is currently using the correct algorithm while and Vector2/Vector4 are not.

It appears that the goal that we are trying to achieve here is two-fold - one being accuracy i.e. ensure that the algorithm is consistent across all three Vector types and returns the correct value (which it wasn't doing earlier due to a floating-point rounding bug) and secondly, allowing optimization code on modern hardware using FusedMultiplyAdd.

That is correct.

The only thing is that I am unable to determine how prevalent the usage is and how many customers would need to update their code to work with this change.

apisof.net currently indicates 0% usage, which doesn't quite seem right (@terrajobst ?):

The change if users are expecting the existing behavior is rather trivial as they can just use v1 + (v2 - v1) * amount; However, I would not suspect anyone is depending on this as Lerp is meant to return exactly v2 if amount is 1 and exactly v1 if amount is 0

@marklio
Copy link

marklio commented May 8, 2020

Unfortunately, apisof.net is notoriously bad for making these kinds of decisions. We're working on democratizing some more helpful data. For now, overall usage of these types is low. Here's the number of NuGet package ids using Lerp from each of the vector* types:

  • System.Numerics.Vector3 19
  • System.Numerics.Vector4 43
  • System.Numerics.Vector2 7

These numbers are pretty low, and we're not breaking them significantly. I agree with @PriyaPurkayastha that this is a fine change for a major release, and we should document it for completeness. My understanding is that they will simply be getting a more accurate result, right? We have precedent for doing that in the JIT without too much concern for compat (we can easily change things that hoist floating point operations to higher precision operations that may yield more accurate results).

@tannergooding
Copy link
Member Author

My understanding is that they will simply be getting a more accurate result, right?

Correct, we would now return exactly value2 rather than almost value2.

@tannergooding tannergooding added help wanted [up-for-grabs] Good issue for external contributors and removed untriaged New issue has not been triaged by the area owner labels Jun 8, 2020
@tannergooding
Copy link
Member Author

I agree with PriyaPurkayastha that this is a fine change for a major release, and we should document it for completeness

I've marked this as up-for-grabs and will prep the breaking change document.

@john-h-k
Copy link
Contributor

john-h-k commented Jun 8, 2020

Can I take this?

@tannergooding
Copy link
Member Author

Assigned out, thanks @john-h-k !

@tannergooding
Copy link
Member Author

Breaking change doc was logged here: dotnet/docs#18937

@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants