Possibility of using the Float trait instead of f64?
#178
Comments
|
My response is basically the same as #159. Generics make the types messier and potentially compile times longer and code more bloated (if there are two versions of the same algorithm). I think when interoperating with other code that prefers f32 (as is the case for Direct2D for example), putting in explicit precision-losing conversions is acceptable. I do see that f64 is more precision than needed for font coordinates. There are other cases, though (scrolling is one) where the lower precision causes problems. If I saw evidence of a real performance difference (as in #159), or evidence that ergonomics would be substantially improved, I would be more inclined to consider it. But in the absence of solid evidence, I am not really in favor. |
|
I remember we did some preliminary benchmarking a few years ago and found there was very little performance advantage to using f64, at least on x86_64. There would of course be some binary size advantages. From where I stand right now, I have a good sense of the downsides of this change, but I don't really have a good sense of the upsides: I mean this in an honest way, I just haven't personally encountered any situations where I've really wished we were using my main concern with this is really the messier types. There's a related argument that kurbo types should include an explicit generic parameter to represent an associated coordinate space; in this way you might have Just to try and think this through, though, if we were going to do one or both of these, what I might imagine is having separate 'Raw' types that have the generic params, and then export simple types that have those params filled in, like: pub struct RawPoint<T, R> {
_space: PhantomData<R>,
pub x: T,
pub y: T,
}
pub trait CoordinateSpace: Copy + Eq {}
// everything is impl'd in terms of these params:
impl<T: CoordinateSpace, R: Float> std::ops::Sub<Point<T, R>> for Point<T, R> {
type Output = Vec2<T, R>;
#[inline]
fn sub(self, other: Point<T, R>) -> Vec2<T, R> {
Vec2::new(self.x - other.x, self.y - other.y)
}
}and then in pub type Point = RawPoint<GenericCoordinateSpace, f64>;And then hopefully the user-facing code looks basically the same as it does now, unless you happen to need to customize. My big problem with this is that I think the docs would end up really gross, and I don't have any idea for how we might address that... |
|
I think that maybe when posits start to be implemented in consumer CPUs there would be a bigger argument for making this generic. However, we are looking 3/4+ years into the future. I myself dont see the point. I just had to debug performance issues related to |
|
I'm very much against making |
|
Couldn't it be optional? Please see |
|
https://docs.rs/integer_or_float/latest/integer_or_float/backing_types/constant.IOF_BACKING_STORE_BITLEN.html It'd be no problem in my understanding to do something like this in kurbo? |
|
Sorry to triple post. Kept thinking of more parallels between the problems. IOF has a lot of feature flags. One is That enables the entire tree under https://docs.rs/crate/integer_or_float/latest/source/src/num_traits_impl/float.rs I use it in MFEK but it can be turned off. |
|
@raphlinus I understand IOF has been rejected for kurbo and norad, no problem. But if it'd help us come to consensus, I can split off the |
|
So I think using a feature to alternate between 32-and-64 bit numbers isn't really an option in a library crate; features are supposed to be additive, not to toggle between alternate incompatible behaviours. This would be a concrete problem: if we imagine a For the latter question, It isn't clear to me what the proposal is? What are we trying to reach consensus on? |
That'd be a crate that is still working only because it has not yet updated to use the |
|
I think the neatest solution is to have a You could create it by cloning the |
|
@derekdreery It would. |
|
We talked about this at the office hours. The consensus was that on CPU, there really isn't much perf benefit to using f32 over f64, so the suggested workflow for people wanting f32 is to convert up when calling kurbo fns, and then cast down ( This doesn't stop anyone from making an |
|
Please reopen this because I'm trying and you're not correct. Tests fail, one loops forever. |
|
Possibly related to #50. |
Can you say more? What are you trying? |
|
The problem is certainly related to choice of epsila. diff --git a/src/common.rs b/src/common.rs
index f4e9107..07944b4 100644
--- a/src/common.rs
+++ b/src/common.rs
@@ -286,7 +286,7 @@ pub fn solve_itp(
) -> f32 {
let n1_2 = (((b - a) / epsilon).log2().ceil() - 1.0).max(0.0) as usize;
let nmax = n0 + n1_2;
- let mut scaled_epsilon = epsilon * (1u64 << nmax) as f32;
+ let mut scaled_epsilon = epsilon * (1u32 << nmax) as f32;
while b - a > 2.0 * epsilon {
let x1_2 = 0.5 * (a + b);
let r = scaled_epsilon - 0.5 * (b - a);
@@ -540,7 +540,7 @@ mod tests {
fn verify<const N: usize>(mut roots: ArrayVec<f32, N>, expected: &[f32]) {
assert_eq!(expected.len(), roots.len());
- let epsilon = 1e-12;
+ let epsilon = f32::EPSILON;
roots.sort_by(|a, b| a.partial_cmp(b).unwrap());
for i in 0..expected.len() {
assert!((roots[i] - expected[i]).abs() < epsilon); |
|
with that patch my CPU thrashing stopped and now i just get failures |
|
All epsila need to be calculated using fred@デブ狸~/Workspace/kurbo% rg 'epsilon[ \t]*='
src/line.rs
301: let epsilon = 1e-9;
// Note: I already changed these in my version but they're wrong in master.
src/common.rs
289: let mut scaled_epsilon = epsilon * (1u32 << nmax) as f32;
543: let epsilon = f32::EPSILON;
src/rounded_rect_radii.rs
96: let epsilon = 1e-9;
src/rounded_rect.rs
441: let epsilon = 1e-9;
473: let epsilon = 1e-7;
src/quadbez.rs
444: let epsilon = 1e-12;
459: let epsilon = 1e-12;
472: let epsilon = 1e-12;
src/cubicbez.rs
744: let epsilon = 1e-12;
755: let epsilon = 1e-12;
817: let epsilon = 1e-12; |
|
If we need to raise them by powers we should multiply |
|
Here's a diff that fixes some affine tests diff --git a/src/affine.rs b/src/affine.rs
index 00d770e..d403e76 100644
--- a/src/affine.rs
+++ b/src/affine.rs
@@ -330,7 +330,7 @@ mod tests {
use std::f32::consts::PI;
fn assert_near(p0: Point, p1: Point) {
- assert!((p1 - p0).hypot() < 1e-9, "{p0:?} != {p1:?}");
+ assert!((p1 - p0).hypot() < f32::EPSILON * (10f32.powi(3)), "{p0:?} != {p1:?}");
}
#[test] |
|
I added this as use core::ops::Mul;
const EPSILON: f64 = f64::EPSILON;
const MAX_10_EXP: i32 = f64::MAX_10_EXP;
/// `kurbo`'s epsilon (ε).
#[repr(packed)]
#[derive(Copy, Clone, Debug)]
pub struct Epsilon {
/// Value of this epsilon. By default, [`f64::EPSILON`].
pub value: f64,
/// Magnitude of this epsilon. By default, [`f64::MAX_10_EXP`].
pub magnitude: i32,
}
impl Default for Epsilon {
#[inline]
fn default() -> Epsilon {
Self::new()
}
}
impl Epsilon {
/// Raises or lowers the magnitude of `kurbo`'s epsilon by `magnitude`, returning a new
/// [`Epsilon`].
///
/// Returns a new `Epsilon` representing **ε × 10_ᵐ_** where _m_ = magnitude.
#[inline]
pub const fn ten_pow(magnitude: i32) -> Self {
debug_assert!(MAX_10_EXP + magnitude <= MAX_10_EXP);
Epsilon {
magnitude: MAX_10_EXP + magnitude,
value: f64::mul(EPSILON, 10.0 * magnitude as f64)
}
}
}
impl Epsilon {
/// Create `kurbo`'s default epsilon.
#[inline]
pub const fn new() -> Self {
Epsilon {
value: EPSILON,
magnitude: MAX_10_EXP
}
}
} |
|
I'd also like kurbo to start declaring a In my version we'll declare that as 32. That's because I will tell people in docs to |
|
@ctrlcctrlv what would the purpose of the pub const be? |
|
Knowing which kurbo I'm using without going through tangled mess of |
|
We talked about this in the office hours again. @raphlinus was concerned that some of the stability analysis of the algorithms would become more important for
For example, if there were use cases like wanting cubic Bézier curves in memory for the GPU, we could add a CubicBez32 struct, where all the algorithms still run in edit tagged Raph because I'm speaking on his behalf and want to check that I'm faithfully representing him. |
|
One possible use of |
Actually, I find this solution perfectly reasonable. Do that. |
This relates to linebender/norad#108.
The
num_traitscrate declares a trait calledFloatwhich implementsNeg,Add,Mul,Rem,Div, etc etc. It then implements this trait forf32andf64.Many libraries which work on numbers use this crate to be generic over floating point types.
I wonder if Kurbo could too?
Edited to add: I think this issue is fundamentally different from, but similar to, #159, because it basically lets the API consumer decide whether or not they are willing to take the risks of
f32lower precision.The text was updated successfully, but these errors were encountered: