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

Consider converting hue types to numbers within [0, 360) instead of (-180, 180] #15

Closed
Ogeon opened this issue Jan 19, 2016 · 4 comments · Fixed by #26
Closed

Consider converting hue types to numbers within [0, 360) instead of (-180, 180] #15

Ogeon opened this issue Jan 19, 2016 · 4 comments · Fixed by #26
Milestone

Comments

@Ogeon
Copy link
Owner

Ogeon commented Jan 19, 2016

Numbers within [0, 360) degrees seems to be more common when working with hues, but the trigonometric functions in Rust gives negative angles instead of angles above 180 degrees. The big question is which one to adopt for our hues. Note that this is only relevant when printing a hue type or when converting it to a "regular" number (e.g. in impl Into<f32>).

@sidred
Copy link
Contributor

sidred commented Jan 19, 2016

In either case we should be consistent internally and externally i.e. do not store it as -180-180 and display it as 0-360. Since the these are public, it will avoid surprises when trying to manually do arithmetic on the hues.

It will be easier to explain in the documentation that hues are -180- 180 than trying to explain the 2 separate ranges.

@Ogeon
Copy link
Owner Author

Ogeon commented Jan 19, 2016

The value is locked inside the *Hue types and you can't do anything with them that makes it matter, which is one of the reasons why they are not just plain f32. What matters to the user is what they look like on the outside. The -180 - 180 range makes it dead easy to find the smallest difference between two hues, so that's why it's sometimes used internally.

@Ogeon Ogeon added this to the 0.2.0 milestone Jan 25, 2016
@Ogeon
Copy link
Owner Author

Ogeon commented Jan 25, 2016

This change should be made as early as possible, if it's should be made at all, so I'm nominating it for the next release.

@Ogeon
Copy link
Owner Author

Ogeon commented Jan 29, 2016

I have decided on a compromise, since both systems are useful in different situations: simply offer both options, but default to the current behavior. The reason for keeping the 0 centered version as the default is that it's consistent with how the trigonometric functions in Rust works. Angle arithmetics are weird in any case.

I'll submit a PR in a moment, with the above mentioned solution. I'll consider this problem resolved when it's merged, since I don't think we can get any further without any actual user feedback.

@homu homu closed this as completed in #26 Jan 29, 2016
homu added a commit that referenced this issue Jan 29, 2016
Offer both 0 centered and positive hue -> float conversion

This is a compromise for the 0 centered vs all positive hue representation problem, where both alternatives are offered, but the current 0 centered behavior is kept as the default. The argument for this decision is that both variants are useful in different situations, and there is no obvious reason why one would be better than the other. The default is kept because it's consistent with how the trigonometric functions in `libstd` works. It's not the strongest reason, but it's as good as any.

Changes in this PR:

 * Rename `to_float` to `to_degrees`.
 * Add `to_positive_degrees` and `to_positive_radians`.
 * Mention the ranges in the descriptions of the conversion functions.

This is a breaking change, since it renames `to_float` to `to_degrees`. It closes #15.
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 a pull request may close this issue.

2 participants