qu1x/miniball

Can `Ball<R, D>` avoid constraining `D: DimNameAdd<U1>` in the struct definition?

Closed this issue · 3 comments

Nice work on this crate! I haven't finished integrating it, but the documentation is really thorough and I was really glad to find someone had implemented Welzl's algorithm with nalgebra types. Thanks for sharing your code!

I've got a minor improvement to suggest. The Ball struct could be nice to work with on its own, as it's a nice convenient representation of a sphere. However, wherever you want to use it, if you are keeping the D dimension generic, you need to have D: DimNameAdd<U1> constraints everywhere. This makes the Ball struct really not worth the bother, you kinda have to just define a new Ball without the Enclosing implementation.

The following code really should work, but it doesn't without adding Const<D>: DimNameAdd<U1> all over the place. That's because for a generic const D: usize, it is not a given that Const<D> will implement DimNameAdd<U1> -- there are a limited range of dimensions that support adding U1, namely Const<0> through Const<127>, or maybe 126, or thereabouts.

// recall the definition in miniball
pub struct Ball<R: RealField, D: DimNameAdd<U1>>
where
	DefaultAllocator: Allocator<R, D>, { ... }

// userland -- I have a ball, I just want to use its contains method or do intersections on it or something
type Ball<const D: usize> = miniball::Ball<f32, D>;
fn use_ball<const D: usize>(ball: Ball<D>) { ... } // error: the trait bound Const<D>: ToTypenum is not satisfied

At the moment, after having constructed a ball using the Enclosing trait, it's much easier not to use miniball::Ball, but rather to define a struct Ball2<const D: usize> { point: SPoint<f32, D>, radius_squared: f32 } and get rid of the constraints.

At the end of the day, if this constraint was removed, Ball would probably need its own contains method that isn't tied up behind the Enclosing trait.

Thanks, glad to here!

Your right, lowring the trait bounds on Ball is quite useful. I've made a commit which also moves the bounds from the Enclosing trait to its methods, e.g. Enclosing::enclosing_points. This makes the Enclosing::contains method available for the lowered bounds as well:

use miniball::{
	nalgebra::{Const, Point},
	Enclosing,
};
use std::collections::VecDeque;

type Ball<const D: usize> = miniball::Ball<f32, Const<D>>;

fn use_ball<const D: usize>(ball: Ball<D>, point: &Point<f32, D>) -> bool {
	ball.contains(point)
}

fn test_contains() {
	const DIM: usize = 1_000;
	let ball = Ball::<DIM> {
		center: Point::origin(),
		radius_squared: 4f32.powi(2),
	};
	assert!(use_ball(ball, &Point::origin()));
}

fn test_enclosing() {
	const DIM: usize = 31; // NOTE: doesn't compile with DIM > 31
	let mut points = VecDeque::<Point<f32, DIM>>::new();
	points.push_back(Point::origin());
	let _ball = Ball::enclosing_points(&mut points);
}

fn main() {
	test_contains();
	test_enclosing();
}

Let me know if it works for you.

Released 0.4.0 with your suggestion implemented, thanks again!

Great, thanks so much!