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.

Sign up for GitHub

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

Jump to bottom

[color] Very generic colors: Rgb, Hsv and Gray #781

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

[color] Very generic colors: Rgb, Hsv and Gray #781

wants to merge 2 commits into from

Conversation

TomSaw
Copy link
Contributor

@TomSaw TomSaw commented Nov 15, 2021 •
edited
Loading

These Colors may be used on their own. F.e. passed to a colored LED driver or being emitted by color-sensor drivers.

They also represent single pixels for modm::graphic::Buffer and modm::graphic::Display (Work in progress, see feature/rewrite-graphic

Features

  • Bit granularity for color::Gray. Same for color::Rgb and color::Hsv cause they're made from 3x color::Gray
    • Proportional conversion between grays with different digits. 2-bit Gray ---> 4-bit Gray f.e. converts as follows:
      • 0b00 --> 0b0000
      • 0b01 --> 0b0101 // No stupid lshift, the gap is filled up
      • 0b10 --> 0b1010
      • 0b11 --> 0b1111 // Saturated stays saturated, white stays white
  • C++20 concepts for colortypes and colorgroups (currently ColorPlane and ColorPalletizing). These concepts support overall readability and enabled very nice overload resolution modulation for modm::graphic::Buffer and modm::graphic::Display (Coming soon, see feature/rewrite-graphic
  • Any imaginable color conversion works - type, granularity, palletized or planar... doesn't matter.
  • Operators with saturation arithmetics

TODO

  • Requires [fix] modm::Saturated #780 being merged
  • Implement some missing operators
  • Optional range check for value in ProportionalUnsigned::ProportionalUnsigned(T value)
  • Update header Timestamps

@TomSaw TomSaw mentioned this pull request Nov 15, 2021
Closed
20 tasks
@TomSaw TomSaw changed the title [ui:color] Very generic colors: Rgb, Hsv and Gray [] Very generic ui:color: Rgb, Hsv and Gray Nov 16, 2021
@TomSaw TomSaw changed the title [] Very generic ui:color: Rgb, Hsv and Gray [color] Very generic colors: Rgb, Hsv and Gray Nov 16, 2021
@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 3 times, most recently from 4bb19da to c5e651d Compare January 9, 2022 13:31
@TomSaw
Copy link
Contributor Author

TomSaw commented Jan 9, 2022 •
edited
Loading

This is ready for a first review @salkinium
I also got my fingers on modm::Saturated again, improved the API and added another test SaturationTest::test__uint8_t_ref.

@salkinium salkinium self-requested a review January 9, 2022 20:26
salkinium
salkinium reviewed Jan 10, 2022
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

So far I like it a lot, very nice design with the generic bit width!

src/modm/math/proportional_unsigned.hpp Outdated Show resolved Hide resolved
src/modm/math/proportional_unsigned.hpp Outdated Show resolved Hide resolved
src/modm/math/saturation/saturated.hpp Outdated Show resolved Hide resolved
src/modm/math/utils/integer_traits.hpp Outdated Show resolved Hide resolved
src/modm/ui/color/concepts.hpp Outdated Show resolved Hide resolved
src/modm/ui/color/gray.hpp Outdated Show resolved Hide resolved
src/modm/ui/color/gray.hpp Outdated Show resolved Hide resolved
src/modm/ui/color/gray.hpp Show resolved Hide resolved
src/modm/ui/color/hsv.hpp Outdated Show resolved Hide resolved
test/modm/math/saturation/saturation_test.hpp Outdated Show resolved Hide resolved
@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 9 times, most recently from 67b645a to 7498e57 Compare January 11, 2022 17:56
salkinium
salkinium reviewed Jan 12, 2022
View reviewed changes
src/modm/math/proportional_unsigned.hpp Outdated Show resolved Hide resolved
src/modm/math/proportional_unsigned.hpp Outdated Show resolved Hide resolved
@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 7 times, most recently from 7d115fe to 4bb51f8 Compare January 12, 2022 14:38
@TomSaw
Copy link
Contributor Author

TomSaw commented Jan 12, 2022 •
edited by salkinium
Loading

Apologize my enduring "push --force" thunderstorm. I'm transitioning from poke mode to test local, push after success mode 😬

Tests passed @salkinium! What's left is a good solution for the ProportionalUnsigned constructor 🙄

@TomSaw
Copy link
Contributor Author

TomSaw commented Jun 22, 2022 •
edited
Loading

Wanna get some stuff off the table. This PR feels robust, if you find a minute @salkinium 😅...

salkinium
salkinium reviewed Jun 22, 2022
View reviewed changes
Copy link
Member

@salkinium salkinium left a comment

Choose a reason for hiding this comment

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

I'm fine with these changes, modulo my doc comment.
I'm two weeks from handing in my thesis, so cannot give you a technical review, but if @chris-durand approves this PR, I'll merge it.

src/modm/ui/color/color.md
Only the Hue component of Hsv ColorType wraps around just like integers do.

## Flexible Widgets
- [ ] Complete this snippet and make the code actually working
Copy link
Member

Choose a reason for hiding this comment

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

?

We generally don't promise future code in the documentation, cos we usually don't get it done in time ;-P

@chris-durand
Copy link
Member

@TomSaw Did you see what I wrote in #781 (comment) ? The discussion got so lengthy that github hides most of it by default 😅

@TomSaw
Copy link
Contributor Author

TomSaw commented Jun 23, 2022

@TomSaw Did you see what I wrote in #781 (comment) ? The discussion got so lengthy that github hides most of it by default 😅

Nope. Totally missed that and check it tomorrow.

@salkinium
Copy link
Member

I'm still very interested in merging this, unfortunately, I've completely lost the overview of the review. I think it's almost done, perhaps you want to address the least comments and then we can merge it?

@TomSaw
Copy link
Contributor Author

TomSaw commented Sep 9, 2023 •
edited
Loading

Hey there. Yes it was almost done. Only chris requested to extract the "arbitrary integer" logic into its own class for good reasons, see #781 (comment) and #781 (comment)

Simple task but then, a strong force pulled me into another universe. One with a ton of PHP actually 🤕

I've just read the processing::fibers readme and it sounds gorgeous 🤩 congrats for completing this!

@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 3 times, most recently from a298341 to 238376e Compare September 10, 2023 07:48
@TomSaw
Copy link
Contributor Author

TomSaw commented Sep 10, 2023

Nothing changed yet.. just cleaned up the tree

@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 13 times, most recently from ab380dc to 7db1732 Compare September 15, 2023 15:18
@TomSaw TomSaw force-pushed the feature/very-generic-colors branch 2 times, most recently from d9f6de5 to 3a9b44e Compare March 4, 2024 09:10
@TomSaw TomSaw force-pushed the feature/very-generic-colors branch from 3a9b44e to eed6979 Compare March 4, 2024 09:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-durand chris-durand chris-durand left review comments

@rleh rleh rleh left review comments

@salkinium salkinium salkinium left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
advanced 🤯 feature 🚧
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@TomSaw @chris-durand @salkinium @rleh