search

fu5ha / sdfu

Signed Distance Field Utilities
https://crates.io/crates/sdfu
118 stars 11 forks source link

Macros for implementing the Vec trait on newtypes? #4

Closed w1th0utnam3 closed 3 years ago

w1th0utnam3 commented 3 years ago

Hey, this is a neat library. I would like to use it for toy sphere tracing project.

However, the dependency on nalgebra is currently a bit outdated. Of course it is easy to update the dependency in a fork or you could update it here but this not really a solution in general. I'm not sure if there is a better solution for this but what do you think about somehow exposing the macros you use for deriving the Vec traits but modifying them such that they implement the trait on newtypes? This way a user would just define e.g.

struct Vector2Wrapper<R: RealField>(pub Vector2<R>);
struct Vector3Wrapper<R: RealField>(pub Vector3<R>);

followed by

impl_vec_nalgebra!(Vector2Wrapper, Vector3Wrapper);

This would allow the user to use any version of the crate without much implementation overhead. I started implementing the trait by hand for the wrappers but this gets really annoying with all the Neg, Mul etc. requirements (of course nalgebra already provides them for Vector3 but I would have to forward everything from the wrapper to nalgebra's implementation).

What are your thoughts on this? If you are open to this idea I can implement it and open a PR. But I'm also not sure what the most idiomatic approach used by other libraries in case of problems like this is.

fu5ha commented 3 years ago

Honestly I think the better approach is to just update the nalgebra dependency to the latest version for now.

The idiomatic approach is to use mint for API boundary types and then just choose one math library to use internally that supports it (I would choose ultraviolet at this point since I wrote it myself and prefer it).

w1th0utnam3 commented 3 years ago

Sounds reasonable!

Interesting, I actually did not know about mint. Sounds like a good approach. Although it's probably a bit annoying for a library like this (where the API surface is relatively large in comparison to the internals?).

I could look into this if I find the time and if you are interested in using this approach for your library. But I also don't want to create a lot of work for you (commenting and reviewing a PR etc.) if you're fine with the current state.

w1th0utnam3 commented 3 years ago

Ah, btw. a bit off topic, but why does SDF require Copy? And would you maybe consider breaking up the current SDF trait into one core SDF part that is only used for evaluation and a ComposableSDF/SDFExt part for the rest? If I'm not mistaken this would allow to compose/transform boxed SDFs (maybe the split is not really necessary for this, would be sufficient to impl the SDF trait for Boxed SDF)? Not that this is desirable for efficient/fast evaluation but then one could quickly hack together an editor that allows to dynamically combine them. To me it looks like this is not possible with the current Copy requirement?

Sorry, don't want to cause too much distraction here if you consider this crate "done" for you.

Edit: Ah, nvm, for an editor one would probably use a different approach as you loose all information with the boxes... still, I wondered about the Copy.

fu5ha commented 3 years ago

Interesting, I actually did not know about mint. Sounds like a good approach. Although it's probably a bit annoying for a library like this (where the API surface is relatively large in comparison to the internals?). I could look into this if I find the time and if you are interested in using this approach for your library. But I also don't want to create a lot of work for you (commenting and reviewing a PR etc.) if you're fine with the current state.

Indeed. I would be fine with reviewing PRs about this, it is what I would do if I was really working with this crate still probably. Just there hasn't really been much interest outside of my own use and I just use ultraviolet so :)

Ah, btw. a bit off topic, but why does SDF require Copy? And would you maybe consider breaking up the current SDF trait into one core SDF part that is only used for evaluation and a ComposableSDF/SDFExt part for the rest? If I'm not mistaken this would allow to compose/transform boxed SDFs (maybe the split is not really necessary for this, would be sufficient to impl the SDF trait for Boxed SDF)? Not that this is desirable for efficient/fast evaluation but then one could quickly hack together an editor that allows to dynamically combine them. To me it looks like this is not possible with the current Copy requirement?

I don't remember exactly why I made it require Copy... I think to make it easier to work with in threaded contexts. I actually had a version of this that was similar to what you are describing in terms of a core SDF and then SDFExt, but the way I did it ended up not working very well. I still think this idea has merit though... the dynamic combination is definitely something that is missing that I think would be very valuable so if you want to explore that realm I would be supportive :)

w1th0utnam3 commented 3 years ago

Ok, thanks for the feedback! Then I'll close this for now and if I have the time and new insights I'll open a new issue for the topic!