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

Add epsilon API #248

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add epsilon API #248

wants to merge 1 commit into from

Conversation

ctrlcctrlv
Copy link

Didn't think there was much point in fixing the epsila until the API is even approved. (See #178.)

@@ -69,6 +69,7 @@
//! [`Druid`]: https://docs.rs/druid
//! [`libm`]: https://docs.rs/libm

#![feature(const_fn_floating_point_arithmetic, const_trait_impl, const_ops)]
Copy link
Author

Choose a reason for hiding this comment

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

note: i can remove these and make the functions non-const but someone here may have better idea

@raphlinus
Copy link
Contributor

We discussed this in office hours today. Generally, seeing this and the back-and-forth in #178, I'm inclined to take this as confirmation that just making everything f64 is the right way to go. Making kurbo generic over precision is not a simple task, and in particular some of the algorithms have very careful numerical robustness work that has to be re-thought. In particular, the work in #230 relies on quartic solving which is very delicate with respect to precision. Making that numerically robust is likely possible (though I'm not sure of that), but would probably be a major effort.

There are use cases where f32 would be very desirable. The biggest is of course if it's running on a GPU (rust-gpu is on my radar). Another is if we're starting to do serious SIMD optimization. However, the costs are nontrivial.

I'd like to understand better exactly what the end goals are. Some goals are probably best met by having all the core algorithms run in f64 but perhaps having conversions to f32 at the edges (this is more or less what we do for Vello scene encoding). Other goals may be met by having, say, an f32 variant of BezPath that implements the Shape trait (we considered this for Vello encoding but ultimately decided against).

Regarding epsilons, we've had discussion on this point before. There's no "one size fits all" approach, and the choice of epsilon depends on the use case. For interactive applications, it's probably best to use the largest value that doesn't cause visual problems, to get the fastest performance. For other applications, it's important to get the right answer. The choice of epsilon can also depend on the range of scaling transforms that are expected and the coordinate system for the final result. Thus, we try not to use "magic constant" epsilon values, and instead pass in an accuracy parameter. It's appropriate to use concrete values in tests (and in general those should be small), as the goal is to detect anything that might cause loss of precision.

Long story short, I am against making a 32 bit version of kurbo unless I see a clear motivation, and until then I am inclined not to accept any changes to facilitate this.

@ctrlcctrlv
Copy link
Author

There are use cases where f32 would be very desirable. The biggest is of course if it's running on a GPU (rust-gpu is on my radar). Another is if we're starting to do serious SIMD optimization. However, the costs are nontrivial.

My code does run on GPU as Skia uses the GPU.

@ctrlcctrlv
Copy link
Author

Thus, we try not to use "magic constant" epsilon values, and instead pass in an accuracy parameter. It's appropriate to use concrete values in tests (and in general those should be small), as the goal is to detect anything that might cause loss of precision.

Primarily that's what this API is for, the tests, as that's where most (all?) of the epsila in the codebase are defined.

@raphlinus
Copy link
Contributor

My code does run on GPU as Skia uses the GPU.

I specifically meant running in shaders or CUDA.

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