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

Port to nalgebra, add methods to assist rendering #6

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dbenson24
Copy link

Hello!
I was looking for a crate to use to create bezier curves in 3D and I really liked your compile time guarantees. I'm opening this PR now mostly just to give you a heads up and brief list of what I've done to see if you are interested in up streaming any of it.

Biggest change upfront is completely removing your Point type in favor of nalgebra's. I think that nalgebra and glam are the two biggest math libraries at this point, and they already have implemented seamless conversions between each other. I think it's less work to just use it.

Next was getting rid of the special implementations of the lower dimensional curves. I just wanted a single type to deal with, and performance shouldn't be different with all the const generic optimizations that can be performed.

The last part I'm working on is creating iterators that break the curve down into line segments so that you can use those line segments to create a mesh, or just draw the lines. I also added the ability to create offset line segments from the curve.

If you're interested in me working on mainstreaming these changes let me know, otherwise I'll probably end up forking.

@dorianprill
Copy link
Owner

dorianprill commented Sep 21, 2022

Hey!
thank you for the PR! I haven't had much time to work on this library lately, but there are many things on my list once I get back to it (hopefully shortly). I am absolutely open to accept PRs and even adding maintainers, if they align with the specific goals of this lib

Biggest change upfront is completely removing your Point type in favor of nalgebra's.

I agree. It always felt like reinventing the wheel but it gave me more control while going through the many iterations of this library.

I think that nalgebra and glam are the two biggest math libraries at this point

What made you choose nalgebra's over glam's ?

Next was getting rid of the special implementations of the lower dimensional curves.

They are mostly a relic of the first development versions. Performance difference should indeed be negligible, especially for the lower order types (and these are usually the ones that are specialized). So yes, that'd be fine.

edit: I'd like to keep them for now - as long as bounding_box() is not supported for generic curves. meanwhile people can use these specialized versions (which is quite common anyway).

The last part I'm working on is creating iterators

This is great help actually. I have only written this lib to backup some knowledge from rather theoretical uni courses (and dabble in const-generics) but have rather little experience with the needs of graphical workloads. So having a more practical API for actual professionals makes a lot of sense and I do need help with that (convenience functions etc). I think naming conventions should align with those of established libraries (e.g. rhino/opennurbs?) where possible.

A quick look over your changes shows you are replacing dependency tinyvec (100% safe rust) with smallvec (a lot of unsafe, kind of a meme in the rust community) which I had also used in the past. This is a change I think I could not compromise on for this library.
At a glance I am not sure what it is required for specifically and would argue it needs to be removed.
I will dive deeper into your PR soon.

Anyway, I expect to be able to again work more on this library the coming months and am happy with any collaboration!

@dbenson24
Copy link
Author

Honestly I swapped to smallvec because I'd used it before. I've got no issues with using tinyvec over it.

The decision to use nalgebra over glam is because nalgebra let's you be generic over the dimension and value type of the vectors, glam does not have any generic typing at all. Glam is often chosen over nalgebra for exactly this reason, because the types are simpler and it compiles a bit faster. Interop between the two is very simple though, they can both be converted into each other use into.

My fn naming is quite bad right now, it's very much in the experimental phase. But I think the most important API for drawing splines is easily breaking it down into line segments. I also wanted the ability to generate offset points so that I could make a mesh out of it

@dorianprill
Copy link
Owner

dorianprill commented Sep 27, 2022

I have to backpedal a bit on the first two points...

  1. I really want to keep the library simple, while making use of rust's powerful trait system. So keeping an own Point trait, which then can be implemented for many external types (nalgbera, glam etc.) and activated as an optional feature in Cargo.toml seems most beneficial. However, implementing it for nalgebra proved to be more difficult than I thought and may force me to rethink my trait bounds (e.g. IntoIterator is not provided by SVector but iter() and iter_mut() are). Keeping a simple type PointN (type and trait could be renamed e.g. Trait: CartesianCoord and Type: Point ) is also nice if you have projects where you don't need all the power of nalgebra's highly generic types (everything is a matrix). Further, nalgebra's SVector and Point behave differently for some arithmetic operation which i need to look into.
    I have a branch nalgebra-compat (not working) where I want to explore this.
  2. As stated in the edit above, these specializations (QuadraticBezier, CubicBezier) should only really be needed until newton-raphson is implemented for the generic versions of the splines to do all sorts of nice things like bounding_boxes, tight boxes. These specializations already support bounding_box() and since they are a common use case for segments, I'd not remove them until then.

I will look into integrating your convenience functions next

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