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

Use RangeInclusive for fNN::lerp #86462

Closed
wants to merge 1 commit into from
Closed

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jun 19, 2021

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

This avoids the question of whether it is (t, a, b) or (a, b, t).
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 19, 2021
@clarfonthey
Copy link
Contributor

clarfonthey commented Jun 19, 2021

Honestly, I should have thought of this API. I like it.

Actually, I think that I've changed my mind mostly because clamp uses the t.f(a, b) calling convention and this would break consistency there.

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.

@m-ou-se
Copy link
Member

m-ou-se commented Jun 19, 2021

This idea was also discussed for the x.clamp(min, max) function (x.clamp(min..=max)), so there's probably some useful context there: #44095

(Edit: Oh, clarfonthey already said that. I missed their edit apparently.)

@scottmcm
Copy link
Member

scottmcm commented Jun 21, 2021

The discussion for clamp might also be on IRLO or the RFC PR -- it was a long conversation.

For me, the persuasive thing was about whether it could reasonably take impl RangeBounds<T> instead of RangeInclusive<T>. I think the answer for that is clearer here than it was for clamp, since x.clamp(ZERO..) has an obvious meaning, but x.lerp(ZERO..) doesn't. So to me that says it shouldn't take RangeInclusive.

(Conveniently, that also means it avoids the "what does it mean to lerp a RangeInclusive that's is_empty problem.)

EDIT: Aha! Found my comment to this affect on the RFC, rust-lang/rfcs#1961 (comment)

@scottmcm scottmcm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jun 21, 2021
@CAD97
Copy link
Contributor Author

CAD97 commented Jun 21, 2021

One of the main arguments for .clamp(min, max) was that (min, max) was consistent with standard practice for clamp methods. Someone seeing x.clamp(a, b) is quite unlikely to think that it's anything other than min, max, because that's so standard.

That argument, at least, doesn't seem to apply as well for lerp. I showed in the tracking issue that all of the somewhat popular math crates in the Rust ecosystem use v0.lerp(v1, t), where this is t.lerp(v0, v1). ultraviolet and vek (the two least downloaded of the ones I looked at) both even provide lerp for f32 with this signature.

Additionally, while Wikipedia says that both lerp(v0, v1, t) and lerp(t, v0, v1) are in use, I don't see the latter much in practice:

HLSL, GLSL, C++ std, Unity, Unreal, Godot, GameMaker, p5.js, and Haskell vector-space provide a free/static function lerp(v0, v1, t).

Godot GDScript (linear_interpolate), Blender Python, and pygame provide v0.lerp(v1, t).

Python numpy (interp) and Haskell linear provide lerp(t, v0, v1).

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 lerp(f32, f32, f32) signature because it does nothing to say what parameter is what, other than convention, and that convention is mixed. t.lerp(v0, v1) appears to be novel, though, and I don't think we would be well served to provide that signature. lerp with a RangeInclusive at the very least avoids the question of parameter order, thus my proposing it for discussion.

That said, lerp very much does want to support inverted ranges, so I understand not putting it on RangeInclusive if that's a deal breaker.

@workingjubilee
Copy link
Member

workingjubilee commented Jun 23, 2021

Very minor weigh-in:
If it's concerns about consistency, I will note that any explicitly vectorized form of lerp, if it wants to be able to lerp to different bounds per lane, would probably prefer an fn lerp(val, val, val) format, in whatever order, as I'm skeptical of the proposed Range*-using type signature efficiently being broken down into and compiling with an array of ranges when it could just use ~3 vectors, and we have tended to prefer consistency with the scalar APIs. I don't think anyone should prioritize this concern above whatever best serves the scalar float APIs, because hypothetically this problem is something that could be fixed in the compiler to "have our cake and eat it too", but it's something I figured I should mention.

As far as t.lerp(v0, v1) being novel, while it is superficially so, it is only because Rust allows writing lerp(t, v0, v1) as that. So here we just imitate Python and Haskell. To the extent that it's weird, I think we're entitled to our eccentricity, and the real question then if we use that may well be "are people going to method-chain producing a float into the first bound v0 or the interpolation value t?"

@scottmcm
Copy link
Member

One thing about t.lerp(v0, v1) that's nice: it extends easily to t.qarp(v0, v1, v2) or t.curp(v0, v1, v2, v3).

@clarfonthey
Copy link
Contributor

That said, lerp very much does want to support inverted ranges, so I understand not putting it on RangeInclusive if that's a deal breaker.

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.

Copy link
Member

@dtolnay dtolnay left a 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).

@dtolnay dtolnay removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 23, 2021
@CAD97
Copy link
Contributor Author

CAD97 commented Jun 27, 2021

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 v0.lerp(v1, t) for f32, but a reasonable unstable period should hopefully allow people to catch that (unstable_name_collision) without silently breaking code. It'll be interesting to see if math libraries adopt this order for vector lerps as well to match std, or stick to their existing order.

@CAD97 CAD97 closed this Jun 27, 2021
@CAD97 CAD97 deleted the lerp-range branch June 27, 2021 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants