bitshifter / glam-rs

A simple and fast linear algebra library for games and graphics
Apache License 2.0
1.46k stars 145 forks source link

Vec2::fract() with negative numbers is inconsistent with f32::fract() #497

Closed caspark closed 4 months ago

caspark commented 4 months ago

fract() (added in #187 to resolve #144) gives different results for negative numbers than std's f32::fract(), which tripped me up.

println!("Float behavior: original: {:?}, fract: {:?}", -0.1f32, -0.1f32.fract());
println!("Vec2 behavior: original: {:?}, fract: {:?}", Vec2::splat(-0.1), Vec2::splat(-0.1).fract());

// output:
//  Float behavior: original: -0.1, fract: -0.1
//  Vec2 behavior: original: Vec2(-0.1, -0.1), fract: Vec2(0.9, 0.9)

The cause is that std implements fract as self - self.trunc() rather than glam's self - self.floor().

However as pointed out in #144, the OpenGL spec indeed says x - x.floor(). (Aside: anyone know why OpenGL defines it that way? Is trunc() slower than floor()?)

Just changing the impl for glam would be a breaking change too since its approach is documented:

/// Returns a vector containing the fractional part of the vector, e.g. `self -
/// self.floor()`.
///
/// Note that this is fast but not precise for large numbers.

Some possible approaches:

cc @hrydgard as original requestor of fract in #144 .

hrydgard commented 4 months ago

I originally requested it for rust-gpu use so consistency with glsl was what I wanted, however since f32::fract isn't consistent, that argument kind of falls.

Deprecating and splitting seems like the way to go to me, though I have no strong opinion here. Clear documentation is of course a must.

bitshifter commented 4 months ago

GLSL is weird. HLSL docs suggest it uses trunc() although their docs are very minimal https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-frac so it's hard to say for certain.

The GLSL implementation doesn't seem useful for negative values but I suspect it's probably used primarily with positive values from textures.

I would be inclined to change the behaviour to match the Rust implementation and document it, might make it a breaking change for 0.27, that's about the best I can do for changes in functionality like this.

bitshifter commented 4 months ago

Of course the first example I find of someone using fract in glsl they're using a sin value as an input. It moves the input into a [0..1] range, so I guess that's got it's uses. Generally glam functions match the Rust function of the same name, although that's not always strictly true, but it probably makes sense to have fract match (f32:f64)::fract and add a fract_gl

bitshifter commented 4 months ago

Fixed in 0.27.0, I'd just pushed a version bump so I though I'd get this in quick and do another version bump before anyone has got around to upgrading to 0.26.0.

caspark commented 4 months ago

That was quick - thank you!