Fix Color.FromHSV hue progression and parameter normalization#9170
Merged
SimonDarksideJ merged 6 commits intoMonoGame:developfrom Jan 16, 2026
Merged
Fix Color.FromHSV hue progression and parameter normalization#9170SimonDarksideJ merged 6 commits intoMonoGame:developfrom
SimonDarksideJ merged 6 commits intoMonoGame:developfrom
Conversation
The fractional position `f` within each 60deg segment was always 0 due to using integer maths.
The documentation for these values state that they are in the [0,1] range. When supplying a value such as 0.5, then the division would do 0.5/100 = 0.005 which is incorrect. `s` and `v` are also linear values in HSV, so clamping makes sense for them. `h` is a cyclical value, so should be allowed to wrap if not in [0,360] range
ThomasFOG
approved these changes
Jan 15, 2026
Contributor
ThomasFOG
left a comment
ThomasFOG
left a comment
There was a problem hiding this comment.
Looking good to me, thanks! Breaking changes are indeed totally fine as this is very new and unrelated to XNA compatibility.
tomspilman
approved these changes
Jan 16, 2026
CartBlanche
pushed a commit
that referenced
this pull request
Jan 22, 2026
## Description Fixes two bugs in `Color.FromHSV()` 1. Fixes fractional hue calculation within color wheel segments. The fractional position `f` within each 60deg segment was calcualted as `int f = (int)(h / 60.0f) - i`, which always resulted in a `0` due to integer arithmetic. Changed to `float f = (h % 60.0f) / 60.0f` to correctly calcualte fractional position within [0,1] range. 2. Removed incorrect normalization of saturation and value parameters. The documentation for the method states that these values should be in the [0,1] range. By normalizing them by dividing by 100, this causes colors to be nearly black in all cases (e.g. passing `s=1.0, v=1.0` would result in RGB values of `2` instead of `255`) Additionally, `Color.FromHSV()`, `Color.FromHSL()` were changed to `static` methods. These are factory methods for creating a `Color` value from a different structure, it doesn't make sense to have them as instance members that require an instance before using. ## Breaking Changes Since `Color.FromHSV()` and `Color.FromHSL()` are changed to static factory methods, this would be considered a breaking change as it changes the public API that already existed. Not sure if you want to keep this change since it's technically a breaking change or we can obsolete the existing ones with a warning and forward them to the new static ones, then remove the instance methods in a future release where breaking changes are good. ## Related Issues - Closes #9131
viniciusjarina
pushed a commit
to codefoco/MonoGame
that referenced
this pull request
Jan 29, 2026
…me#9170) ## Description Fixes two bugs in `Color.FromHSV()` 1. Fixes fractional hue calculation within color wheel segments. The fractional position `f` within each 60deg segment was calcualted as `int f = (int)(h / 60.0f) - i`, which always resulted in a `0` due to integer arithmetic. Changed to `float f = (h % 60.0f) / 60.0f` to correctly calcualte fractional position within [0,1] range. 2. Removed incorrect normalization of saturation and value parameters. The documentation for the method states that these values should be in the [0,1] range. By normalizing them by dividing by 100, this causes colors to be nearly black in all cases (e.g. passing `s=1.0, v=1.0` would result in RGB values of `2` instead of `255`) Additionally, `Color.FromHSV()`, `Color.FromHSL()` were changed to `static` methods. These are factory methods for creating a `Color` value from a different structure, it doesn't make sense to have them as instance members that require an instance before using. ## Breaking Changes Since `Color.FromHSV()` and `Color.FromHSL()` are changed to static factory methods, this would be considered a breaking change as it changes the public API that already existed. Not sure if you want to keep this change since it's technically a breaking change or we can obsolete the existing ones with a warning and forward them to the new static ones, then remove the instance methods in a future release where breaking changes are good. ## Related Issues - Closes MonoGame#9131
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes two bugs in
Color.FromHSV()fwithin each 60deg segment was calcualted asint f = (int)(h / 60.0f) - i, which always resulted in a0due to integer arithmetic. Changed tofloat f = (h % 60.0f) / 60.0fto correctly calcualte fractional position within [0,1] range.s=1.0, v=1.0would result in RGB values of2instead of255)Additionally,
Color.FromHSV(),Color.FromHSL()were changed tostaticmethods. These are factory methods for creating aColorvalue from a different structure, it doesn't make sense to have them as instance members that require an instance before using.Breaking Changes
Since
Color.FromHSV()andColor.FromHSL()are changed to static factory methods, this would be considered a breaking change as it changes the public API that already existed. Not sure if you want to keep this change since it's technically a breaking change or we can obsolete the existing ones with a warning and forward them to the new static ones, then remove the instance methods in a future release where breaking changes are good.Related Issues