Skip to content

Make Lerp private and fix range issue#2998

Open
RunDevelopment wants to merge 1 commit into
image-rs:mainfrom
RunDevelopment:lerp-private
Open

Make Lerp private and fix range issue#2998
RunDevelopment wants to merge 1 commit into
image-rs:mainfrom
RunDevelopment:lerp-private

Conversation

@RunDevelopment
Copy link
Copy Markdown
Member

Addresses L13 from #2954.

Changes:

  • Make Lerp crate private using primitive sealed.
  • Always use f32 as the ratio for simplicity.
  • Fixed that clamping was done with a min of 0, which is incorrect for signed primitives.

Comment thread src/primitive_sealed.rs
}
)+ };
}
impl_lerp_with_f64_int!(u32, u64, usize, i32, i64, isize);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't really lerp 64-bit integer types, including usize, with f64. You'll already have a loss of precision in the initial cast as the mantissa is too small, and unlike for blurring this is highly unexpected. (E.g. it breaks the proposition that the result is contained in the interval between the two endpoints for ratio in [0.0, 1.0]). I think the PR should stick to what was there.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll already have a loss of precision in the initial cast as the mantissa is too small

Sure, but I don't think that's a huge issue. Only two functions use lerp anyway.

I think the PR should stick to what was there.

What do you mean here? "What was there" was that these types didn't implement lerp. That's not an option for a sealed trait. So do you want to keep Lerp public and just fix the clamping?

Copy link
Copy Markdown
Member

@197g 197g May 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, we can't provide those traits for only some of the internal types. That's rather ugly, it should not just panic either as it is probably accessible publicly through some paths. Ugh. Note sure then, I'll ponder it a bit.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want an idea for something that doesn't lose precision, I have one.

Ratio is defined as, well, a ratio. In particular, it's the ratio of 2 u32s. So we could change the API to take the ratio as 2 u32s. I.e. fn lerp(a: Self, b: Self, ratio: (u32, u32)) -> Self. Then the implementation can decide what to do with it. In particular, higher-bit integer types can use integer division. E.g. here's u64:

fn lerp(a: u64, b: u64, ratio: (u32, u32)) -> u64 {
    let a: i128 = a as i128;
    let b: i128 = b as i128;
    let res = a + ((b - a) * ratio.0 as i128 + (ratio.1 / 2 as i128)) / ratio.1 as i128;
    res as u64
}

Kind of ugly, but it works.

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.

2 participants