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

Rounding of value and chroma when using munsell2rgb #44

Closed
pierreroudier opened this issue Jan 18, 2018 · 5 comments
Closed

Rounding of value and chroma when using munsell2rgb #44

pierreroudier opened this issue Jan 18, 2018 · 5 comments

Comments

@pierreroudier
Copy link
Collaborator

At the moment, munsell2rgb requires the_value and the_chroma to be integers, otherwise a NA is returned:

> aqp::munsell2rgb(the_hue = '10YR', the_value = 3.9, the_chroma = 5.4)
[1] NA
> aqp::munsell2rgb(the_hue = '10YR', the_value = 3, the_chroma = 5 )
[1] "#614218FF"

I think this should be made clear(er) to users. Different options:

  • Specifying it more explicitly in the documentation
  • Get munsell2rgb to automatically convert the_value and the_chroma to integers, with a warning
  • Get munsell2rgb to throw an error if the_value and/or the_chroma are not integers
@dylanbeaudette
Copy link
Member

Correct. munsell2rgb is based only on conversion via lookup table.

All good points: the manual should make it more clear and offer a solution for non-standard Munsell notation, such as 8.9YR 3.9/5.4.

For example:

library(aqp)

# troublesome color, probably generated via conversion from other color space
m <- '8.9YR 3.69/5.4'

# parse into pieces, don't attempt conversion
(mp <- parseMunsell(m, convertColors = FALSE))

# conversion doesn't work because it is performed via LUT
aqp::munsell2rgb(mp$hue, mp$value, mp$chroma)

# you can find the closest Munsell "chip" in the LUT
getClosestMunsellChip(m, convertColors = FALSE)

# convert the closest "chip" into sRGB
getClosestMunsellChip(m, convertColors = TRUE, return_triplets=TRUE)

@dylanbeaudette
Copy link
Member

Another idea, perhaps for aqp 2.0: make munsell2rgb more intelligent so as to invoke getClosestMunsellChip as needed.

Also, see #10 for the original issue on non-standard Munsell notation.

@pierreroudier
Copy link
Collaborator Author

pierreroudier commented Jan 18, 2018

Right,

I've whipped up a naive patch:

# If the_value or the_chroma are not integers, round them
  if ( !isTRUE(all.equal(the_value, as.integer(the_value) )) ) {
    the_value <- round(the_value)
    warning("'the_value' has been rounded to the nearest integer.")
  }
  if ( !isTRUE(all.equal(the_chroma, as.integer(the_chroma) )) ) {
    the_chroma <- round(the_chroma)
    warning("'the_chroma' has been rounded to the nearest integer.")
  }

See b1fdc73

But maybe invoking getMunsellMunsellChip would be better.

pierreroudier added a commit that referenced this issue Jan 18, 2018
@dylanbeaudette
Copy link
Member

This works for now. For next time:

  1. finish Munsell color conversions #10 with full interpolation of non-standard Munsell notation in CIE LAB space
  2. integrate into munsell2rgb as an option: round or interpolate to closest chip

@dylanbeaudette
Copy link
Member

For some reason b1fdc73 didn't make it into the code. I've added a slightly modified version and related tests.

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

No branches or pull requests

2 participants