Add parsing color_t #2015

VittorioSanchez · 2025-02-24T15:31:54Z

Added parsing for:

#RRGGBB
rgb(R, G, B)
hsl(H, S%, L%)
hsv(H, S%, V%)
hwb(H, W%, B%)
cmyk(C%, M%, Y%, K%)
CSS color names

These are case insensitive and whitespace insensitive. % symbol is also optional.
Limits are checked and throws error if out of range.
Updated doc and added tests

Fix: #2003

library/private/options_tools.h

codecov · 2025-02-27T15:08:42Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.82%. Comparing base (9dbb696) to head (610e379).
Report is 12 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2015      +/-   ##
==========================================
+ Coverage   95.79%   95.82%   +0.02%     
==========================================
  Files         128      128              
  Lines       10396    10560     +164     
==========================================
+ Hits         9959    10119     +160     
- Misses        437      441       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

library/private/options_tools.h

snoyer

some nitpicks as per tradition but look good :)

mwestphal · 2025-03-05T18:45:41Z

@VittorioSanchez Is this ready for review ? :)

VittorioSanchez · 2025-03-06T07:14:06Z

@VittorioSanchez Is this ready for review ? :)

Yes it is

doc/user/PARSING.md

library/private/options_tools.h

doc/user/PARSING.md

library/private/options_tools.h

mwestphal

nice work, a few questions though

mwestphal · 2025-03-08T07:30:17Z

Please resolve discussions you have adressed and ping me and @snoyer for a review when ready @VittorioSanchez :)

doc/user/PARSING.md

library/private/options_tools.h

snoyer · 2025-03-08T09:49:57Z

the regex_search("^...$") calls can be replaced with regex_match("...")

library/private/options_tools.h

mwestphal · 2025-03-08T20:48:18Z

doc/user/PARSING.md

+
+See [W3C](https://www.w3.org/TR/css-color-3/#rgb-color) doc for more details on these formats.
+
+When formatting a color into a string, it is formatted as `#RRGGBB` if values are multiple of 255. Otherwise, it is formatted as `R,G,B` where R, G, B are doubles between 0 and 1.


Suggested change

When formatting a color into a string, it is formatted as `#RRGGBB` if values are multiple of 255. Otherwise, it is formatted as `R,G,B` where R, G, B are doubles between 0 and 1.

When formatting a color into a string, it is formatted as `#RRGGBB` if values are multiple of 255. Otherwise, it is formatted as vector of doubles.

mwestphal · 2025-03-08T20:49:23Z

doc/user/PARSING.md

-Color are parsed and formatted as a vector of double.
+The following formats are supported when parsing a color, case insensitive:
+
+- R,G,B where R, G, B are doubles [0, 1]


Suggested change

- R,G,B where R, G, B are doubles [0, 1]

- R,G,B where R, G, B are doubles >= 0

mwestphal · 2025-03-08T20:58:05Z

library/private/options_tools.h

+    }
+
+    /* Named colors search */
+    vtkSmartPointer<vtkNamedColors> color = vtkSmartPointer<vtkNamedColors>::New();


Suggested change

vtkSmartPointer<vtkNamedColors> color = vtkSmartPointer<vtkNamedColors>::New();

vtkNew<vtkNamedColors> color;

mwestphal · 2025-03-08T21:00:09Z

library/private/options_tools.h

+
+    /* Vector double format */
+    std::vector<double> vecColor = options_tools::parse<std::vector<double>>(str);
+    if (std::any_of(vecColor.begin(), vecColor.end(), [](double value) { return !(value >= 0.0); }))


I wonder if we should remove this check. Anyone using the struct API can directly put negative values in anyway so its not like we can then skip checks later on, and it restrict a bit what the user can do.

If it makes sense to someone to input negative values, it may be simpler to let them (and catch the error later when we use the value, which we already do)

mwestphal · 2025-03-08T21:00:24Z

library/private/options_tools.h

  }
+  /* We do not catch std::invalid_argument exception from stod as it is covered by the regex */


nor std::out_of_range

mwestphal · 2025-03-08T21:02:34Z

library/private/options_tools.h

-  return options_tools::format(static_cast<std::vector<double>>(var));
+  const std::vector<double> colors = { var.r(), var.g(), var.b() };
+  if (std::all_of(colors.begin(), colors.end(),
+        [](double val) { return (val >= 0 && val <= 1 && std::fmod(val * 255., 1) < 1e-9); }))


does using std::epsilon makes sense here ?

mwestphal · 2025-03-08T21:03:17Z

library/testing/TestSDKOptionsIO.cxx

@@ -110,8 +110,39 @@ int TestSDKOptionsIO(int argc, char* argv[])

  test.parse<f3d::color_t>("color_t", "0.1,0.2,0.3", { 0.1, 0.2, 0.3 });
  test.parse<f3d::color_t>("color_t", "  0.1,  0.2 , 0.3 ", { 0.1, 0.2, 0.3 });
+  test.parse<f3d::color_t>("color_t", "#FFFFFF", { 1.0, 1.0, 1.0 });


looking at w3c, it looks like they also list #FFF as supported format. Does that work ? If not is it easy to add ?

mwestphal · 2025-03-08T21:04:55Z

library/testing/TestSDKOptionsIO.cxx

  test.parse_expect<f3d::color_t, parsing_exception>("incorrect size color_t", "0.1,0.2,0.3,0.4");
+  test.parse_expect<f3d::color_t, parsing_exception>("invalid color_t", "-0.1,-0.2,-0.3");
+  test.parse_expect<f3d::color_t, parsing_exception>(
+    "out of range color_t", std::string(outOfRangeDoubleStr + ",0.2,0.3"));


Im not following this test ?

mwestphal

some questions and remarks

snoyer reviewed Feb 24, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Feb 24, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Feb 24, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Feb 24, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

VittorioSanchez marked this pull request as ready for review February 27, 2025 14:44

VittorioSanchez changed the title ~~[DRAFT] Add parsing color_t~~ Add parsing color_t Feb 27, 2025

VittorioSanchez force-pushed the parsing_color_t branch from 86a9843 to 29a20f3 Compare February 27, 2025 14:46

snoyer reviewed Feb 27, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

mwestphal mentioned this pull request Mar 1, 2025

Add proper parsing for color_t #2003

Open

VittorioSanchez added 11 commits March 3, 2025 17:08

Add rgb and hex parsing

4607493

Add named colors parsing

7805459

Fix after review

41d7a05

Added hsv parsing

6b563f6

Add hsl parsing

5a9c684

Add hwb parsing

1fbbc3c

Refactor hue based regex search

03f4d5f

Add cmyk parsing

c2d5673

Robustified and cleaned

8f4a4ab

Updated color parsing doc

f5881be

Fix wring var name

9791116

VittorioSanchez force-pushed the parsing_color_t branch from f75e84a to 4ae7f9b Compare March 3, 2025 16:19

Fix regex and coverage after review

eaf2963

VittorioSanchez force-pushed the parsing_color_t branch from 4ae7f9b to eaf2963 Compare March 3, 2025 16:27

Fix prettier formating

96e6fcc

VittorioSanchez requested a review from snoyer March 4, 2025 13:23

snoyer reviewed Mar 4, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Mar 4, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Mar 4, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Mar 4, 2025

View reviewed changes

mwestphal reviewed Mar 6, 2025

View reviewed changes

doc/user/PARSING.md Outdated Show resolved Hide resolved

mwestphal reviewed Mar 6, 2025

View reviewed changes

doc/user/PARSING.md Outdated Show resolved Hide resolved

mwestphal reviewed Mar 6, 2025

View reviewed changes

library/private/options_tools.h Show resolved Hide resolved

mwestphal reviewed Mar 6, 2025

View reviewed changes

library/private/options_tools.h Show resolved Hide resolved

mwestphal reviewed Mar 6, 2025

View reviewed changes

doc/user/PARSING.md Outdated Show resolved Hide resolved

mwestphal reviewed Mar 6, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

mwestphal requested changes Mar 6, 2025

View reviewed changes

VittorioSanchez added 2 commits March 7, 2025 17:45

Fix after review

8d4777a

Added color_t hex format string

69777dd

mwestphal reviewed Mar 8, 2025

View reviewed changes

doc/user/PARSING.md Outdated Show resolved Hide resolved

mwestphal reviewed Mar 8, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

mwestphal reviewed Mar 8, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

snoyer reviewed Mar 8, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

VittorioSanchez commented Mar 8, 2025

View reviewed changes

library/private/options_tools.h Outdated Show resolved Hide resolved

Fix after review

610e379

VittorioSanchez requested review from mwestphal and snoyer March 8, 2025 14:53

mwestphal reviewed Mar 8, 2025

View reviewed changes

mwestphal requested changes Mar 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add parsing color_t #2015

Add parsing color_t #2015

VittorioSanchez commented Feb 24, 2025 •

edited by mwestphal

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

snoyer left a comment

mwestphal commented Mar 5, 2025

VittorioSanchez commented Mar 6, 2025

mwestphal left a comment

mwestphal commented Mar 8, 2025

snoyer commented Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal Mar 8, 2025

mwestphal left a comment


		See [W3C](https://www.w3.org/TR/css-color-3/#rgb-color) doc for more details on these formats.

		When formatting a color into a string, it is formatted as `#RRGGBB` if values are multiple of 255. Otherwise, it is formatted as `R,G,B` where R, G, B are doubles between 0 and 1.

	- R,G,B where R, G, B are doubles [0, 1]
	- R,G,B where R, G, B are doubles >= 0

	vtkSmartPointer<vtkNamedColors> color = vtkSmartPointer<vtkNamedColors>::New();
	vtkNew<vtkNamedColors> color;

		}
		/* We do not catch std::invalid_argument exception from stod as it is covered by the regex */

Add parsing color_t #2015

Are you sure you want to change the base?

Add parsing color_t #2015

Conversation

VittorioSanchez commented Feb 24, 2025 • edited by mwestphal Loading

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

snoyer left a comment

Choose a reason for hiding this comment

mwestphal commented Mar 5, 2025

VittorioSanchez commented Mar 6, 2025

mwestphal left a comment

Choose a reason for hiding this comment

mwestphal commented Mar 8, 2025

snoyer commented Mar 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mwestphal left a comment

Choose a reason for hiding this comment

VittorioSanchez commented Feb 24, 2025 •

edited by mwestphal

Loading

codecov bot commented Feb 27, 2025 •

edited

Loading