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

Addressing color palette issue in issue #34 #36

Closed

Conversation

nanobowers
Copy link
Contributor

Some support for 256 color mode was allowed in the styled_printer.rb, however it could not be fully accessed.
I have updated the code to allow for a numeric value of a color (0..255) to be used, or a symbol.

Some things that i have found doing this:

  • The normal use for coloring is to use a named color (e.g. color: :blue). Definitions for the light colors (e.g. :light_blue) and black exist, but are not accessible to plotting. I am unsure if this is a bug or feature.
  • The normal plotting case uses a binary-OR function | to OR together the color's number for a given screen element. This is a nice effect for 8-color mode (e.g. can blend blue and yellow to make green), but this may not give a desired result for 256 color mode (e.g. color 0b11110000 and color 0b00001111 will blend to color 0b11111111)
  • support for 24b color is not supported in the existing code or part of this gem. if this is desired, i would recommend using something like the 'paint' gem, though this will add a dependency.

I believe this will fix issue #34 from @kojix2 , but If there is a desire to support the full range of the basic 16 ANSI color palette or blending is needed for the 256 color palette then we will need to consider more complex methods to handle it.

@@ -0,0 +1,39 @@
#!/bin/env ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an example script to show usage of 256-color palette and 8-color (with mixing).

@@ -46,7 +46,7 @@ def pixel!(pixel_x, pixel_y, color)
index = index_at(char_x - 1, char_y - 1)
if index
@grid[index] = (@grid[index].ord | BRAILLE_SIGNS[char_x_off - 1][char_y_off - 1]).chr(Encoding::UTF_8)
@colors[index] |= COLOR_ENCODE[color]
@colors[index] |= COLOR_ENCODE.fetch(color, color)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is that if color is a symbol (e.g. :blue), then it will fetch a number. If color is a number it will fail to find in the Hash, and just pass the color through.

@nanobowers nanobowers mentioned this pull request Jan 1, 2021
@mrkn
Copy link
Member

mrkn commented Jan 14, 2021

First, thank you for your pull-request. I consider how to handle this and I decided that I don't merge this.

My consideration is below:

  • In BrailleCanvas#plot!, color argument must be a Symbol. This restriction comes from UnicodePlot.jl. I don't think this is a bug because UnicodePlot.jl explicitly restrict the type of the argument.
  • In 256-color mode, we should decode color ID to RGB components, blend colors, and decode it back to color ID.
  • If we accept to use color ID, it should be implemented at the same time as full-color support.

@kojix2
Copy link
Member

kojix2 commented Jan 14, 2021

I don't know what it is, but it's a pity I can't use a lot of colors...

Never mind. This is just a statement of my feelings. Not about the coding.

@mrkn
Copy link
Member

mrkn commented Jan 14, 2021

At least @colors[index] |= COLOR_ENCODE.fetch(color, color) isn't acceptable to me. Even if this change is merged, you cannot use many colors for every case.

@nanobowers
Copy link
Contributor Author

@mrkn I agree with your assessment. This fix was kind of a hack, and is not a good long term solution. I will close this request.

To fix this properly will require better support for 256 color mode (and true 24bit) which is more complex and will take some time. Sorry to get your hopes up, @kojix2 - i will try to look at that again when i am finished working on #30

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.

3 participants