BoxDragon / kolor

Color space conversion math made simple
38 stars 7 forks source link

For your consideration: Remove `glam` dependency. #4

Closed kettle11 closed 3 years ago

kettle11 commented 3 years ago

This implements a very minimalist version of Vec3, Mat3, and Vec2 as a replacement for the full glam dependency.

This implementation is very spare: presently it only implements the exact functions used by kolor.

Pros:

Cons:

Let me know what you think!

Edit: I forgot to add that I wasn't able to verify that there are no test regressions because a few tests were already failing onmain.

kabergstrom commented 3 years ago

This is great! Thank you for the PR, this was something that was on my TODO list for sure. I think type compatibility can be somewhat solved with mint. I was also thinking to remove Color from the API, and make ColorConversion the primary type, in order to minimize the API surface.

Regarding tests, it's definitely something I haven't built out properly, shame on me. I want to use the python colour-science package to generate test data to use as a reference.

May I ask how you found the project?

kettle11 commented 3 years ago

May I ask how you found the project? I'm often in the Game Development in Rust server (I go by kettlecorn there). I saw your presentation about kolor and we briefly chatted about it after. I've had a mental "to-do" to try kolor out since.

Glad you like the PR! Let me know if there's any changes you want to see before merging.

kabergstrom commented 3 years ago

I'm often in the Game Development in Rust server (I go by kettlecorn there). I saw your presentation about kolor and we briefly chatted about it after. I've had a mental "to-do" to try kolor out since.

Oh cool! I didn't realize you're kettlecorn.

Glad you like the PR! Let me know if there's any changes you want to see before merging.

Mostly looks good, there are a few printlns left which would be nice to get rid of. Also please cargo run the matrix-gen command to regenerate matrices and commit those too. I pulled your changes and tried it, and a few decimals changed but idk if it matters much, or if the glam implementation was more or less correct. I think the matrix inverse is what introduces a lot of potential error, generally.

kettle11 commented 3 years ago

Mostly looks good, there are a few printlns left which would be nice to get rid of.

Fixed! Sorry about that!

Also please cargo run the matrix-gen command to regenerate matrices and commit those too

Done!

I pulled your changes and tried it, and a few decimals changed but idk if it matters much, or if the glam implementation was more or less correct. I think the matrix inverse is what introduces a lot of potential error, generally.

Yeah I suspect it's just based on the order operations are performed. Potentially certain orderings could be slightly more correct, but it seems unlikely it'd be meaningfully different.

kabergstrom commented 3 years ago

Thank you very much!