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

use flt() function #35

Merged
merged 1 commit into from
Feb 2, 2016
Merged

use flt() function #35

merged 1 commit into from
Feb 2, 2016

Conversation

sidred
Copy link
Contributor

@sidred sidred commented Feb 2, 2016

Use function flt() to convert constants to trait Float type. Changed all T::from().unwrap() to use this flt() function defined in lib.rs. Makes the code easier to read, especially the formula's.

closes #33


use palette::{Rgba, Gradient, Mix};
use palette::{Rgba, Gradient, Mix, flt};
Copy link
Owner

Choose a reason for hiding this comment

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

Oh, I don't think we should make it public...

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not even sure why the function below uses T instead of f32.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Damn, I was aiming for a no error merge. May be next time :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Owner

Choose a reason for hiding this comment

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

Haha, it was almost flawless 😄 This is why it's nice to have a second pair of eyes that can review ones changes. I don't know if you have considered it, but you are most welcome to review my pull requests, as well. You have full permission to use my own rules and guidelines against me 😉

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

Did you try to apply it in gradient.rs?

@sidred
Copy link
Contributor Author

sidred commented Feb 2, 2016

I missed those. Changes done

@@ -22,10 +24,10 @@ impl<C: Mix + Clone> Gradient<C> {
pub fn new<I: IntoIterator<Item = C>>(colors: I) -> Gradient<C> {
let mut points: Vec<_> = colors.into_iter().map(|c| (C::Scalar::zero(), c)).collect();
assert!(points.len() > 0);
let step_size = C::Scalar::one() / <C::Scalar as NumCast>::from(max(points.len() - 1, 1) as f64).unwrap();
let step_size = C::Scalar::one() / flt::<C::Scalar, _>(max(points.len() - 1, 1) as f64);
Copy link
Owner

Choose a reason for hiding this comment

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

Was it unable to infer C::Scalar?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I assumed it wouldn't, but it does work. Made the change.

@sidred
Copy link
Contributor Author

sidred commented Feb 2, 2016

Yup, missed a couple. Everything in this file should be ok now.

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

Great! It's so nice to read this diff. The new code is so much tidier. I think this was all, so let's run it through the compactor and merge.

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

...and it's even negative. You managed to shorten the code by one line.

@sidred
Copy link
Contributor Author

sidred commented Feb 2, 2016

done

@Ogeon
Copy link
Owner

Ogeon commented Feb 2, 2016

Alright, thank you!

@homu r+

@homu
Copy link
Contributor

homu commented Feb 2, 2016

📌 Commit eb7b027 has been approved by Ogeon

@homu
Copy link
Contributor

homu commented Feb 2, 2016

⚡ Test exempted - status

@homu homu merged commit eb7b027 into Ogeon:master Feb 2, 2016
homu added a commit that referenced this pull request Feb 2, 2016
use flt() function

Use function flt() to convert constants to trait Float type. Changed all T::from().unwrap() to use this flt() function defined in lib.rs. Makes the code easier to read, especially the formula's.

closes #33
@sidred sidred deleted the use_float_fn branch February 2, 2016 14:48
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.

use macros for float conversions
3 participants