-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Use RangeInclusive for fNN::lerp #86462
Conversation
This avoids the question of whether it is (t, a, b) or (a, b, t).
r? @dtolnay (rust-highfive has picked a reviewer for you, use r? to override) |
Actually, I think that I've changed my mind mostly because Plus, there's a bit of uncertainty here with what you should do if the list is empty, since that's possible for inclusive ranges. |
This idea was also discussed for the (Edit: Oh, clarfonthey already said that. I missed their edit apparently.) |
The discussion for For me, the persuasive thing was about whether it could reasonably take (Conveniently, that also means it avoids the "what does it mean to lerp a EDIT: Aha! Found my comment to this affect on the RFC, rust-lang/rfcs#1961 (comment) |
One of the main arguments for That argument, at least, doesn't seem to apply as well for Additionally, while Wikipedia says that both HLSL, GLSL, C++ Godot GDScript ( Python numpy ( I've obviously omitted many other major math libraries, but only have so much time and space. Rust libraries are covered in the tracking issue. To that end, I've now dug myself into a deeper whole on deciding what signature to use. I just really dislike the That said, |
Very minor weigh-in: As far as |
One thing about |
I do think that it's a deal-breaker, and any version of lerp that doesn't allow it would be very surprising. As mentioned I think in the docs for the method, lerp is extremely important for smooth transitions between values, and you can transition between any values, regardless of direction. A clamped lerp is debatable (personally, I think that users should do their own clamping), but only allowing increasing ranges seems bad imho. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be to stick with t.lerp(v0, v1)
. #86462 (comment) is compelling, and to a lesser extent #86462 (comment).
I agree with @dtolnay here, so I'm closing this. I opened the PR to get discussion on the API choice for this method, and got that discussion, so it's served its purpose. I still worry about existing library impls that provide |
This avoids the question of whether it is
(t, a, b)
or(a, b, t)
by making it(t, a..=b)
.Allows inverted ranges, which may need discussion.
Tracking issue: #86269
cc @clarfonthey