Add base color error msgs by NoSloppy · Pull Request #844 · profezzorn/ProffieOS

NoSloppy · 2025-08-16T02:15:02Z

Just compile time error messages that are user friendly.

profezzorn · 2025-08-16T02:20:33Z

styles/layers.h

+};
+
+// Solid+solid check
+template<class BASE, class L1>


Isn't it true that no layer except the first one should be solid?

hmm. would "“Reject solid layers after the first is found" be a better comment?
This would go away anyway with a move to color.h

Comment updated.

I'm saying that we don't need to make so many changes, we can just insert the assert into Compose<>
Basically, if the second argument to Compose is opaque -> error

profezzorn · 2025-08-16T02:21:41Z

styles/layers.h

+#include <utility>
+
+// Identify “solid” colors by the return type of getColor()
+namespace layers_detail {


This seems like it could be useful in lots of places, so maybe call it color_details and put it in common/color.h instead?

styles/style_ptr.h

profezzorn · 2025-08-16T02:25:04Z

styles/style_ptr.h

+  template<class T> struct TopBase { using type = T; };
+  template<class B, class L> struct TopBase<Compose<B,L>> : TopBase<B> {};
+
+  template<typename T> struct IsOpaqueColor : std::false_type {};


This is the same code as in layers_details, so it should definitely go in common/color.h

And make messages obnoxiously visible so they are easy to find in a sea of "ambiguous overload" errors.

NoSloppy · 2025-08-16T04:24:02Z

I also made messages obnoxiously visible so they are easy to find in a sea of "ambiguous overload" errors.

profezzorn · 2025-08-16T04:37:33Z

styles/layers.h

+};
+
+// Solid+solid check
+template<class BASE, class L1>


I'm saying that we don't need to make so many changes, we can just insert the assert into Compose<>
Basically, if the second argument to Compose is opaque -> error

styles/style_ptr.h

also cover scenarios for Black and Alpha 0 being first

profezzorn · 2025-08-16T08:03:20Z

styles/layers.h

 public:
+  using _BaseColorT  = decltype(std::declval<BASE&>().getColor(0));
+  using _LayerColorT = decltype(std::declval<L1&>().getColor(0));
+  static_assert(!(color_details::IsOpaqueColor<_BaseColorT>::value &&


I think it's enough to just check if _LayerColorT is opaque.
It doesn't matter if _BaseColorT is opaque or not.

styles/style_ptr.h

profezzorn · 2025-08-16T21:17:02Z

styles/layers.h

 public:
+static_assert(!color_details::IsOpaqueColor<decltype(std::declval<L1&>().getColor(0))>::value,
+                "\n\n"
+                "*** Layers<> ERROR: CANNOT STACK TWO SOLID COLORS.\n\n");


This error needs updating now.
Something like "Layers<> error: Only the base color may be solid" maybe?

profezzorn · 2025-08-16T21:17:16Z

styles/layers.h

 template<class BASE, class L1>
 class Compose {
 public:
+static_assert(!color_details::IsOpaqueColor<decltype(std::declval<L1&>().getColor(0))>::value,


Should be indented two more steps.

Doesn't look fixed.
Note, I'm talking about the static_assert itself, not the string inside it.

profezzorn · 2025-08-16T21:23:23Z

styles/layers.h

+// Drop fully transparent first item (Alpha 0) when there are exactly 2 args.
 template<class BASE, class L1> struct LayerSelector<AlphaL<BASE, Int<0>>, L1> { typedef L1 type; };
+
+// Drop fully transparent first item (Alpha 0) when there are 3+ args.


I think the correct fix for this is to change line 82 from:

typedef typename LayerSelector<Compose<BASE, L1>, REST...>::type type;

to

typedef typename LayerSelector<LayerSelector<BASE, L1>::type, REST...>::type type;

I think that's what I originally intended anyways.
That way everything gets broken down into two-layer calls to LayerSelector, and we don't have to special-case three-layer stuff.

ok cool, done.

I think that means we don't need this 3-layer case anymore.

oops. actually deleted this time.

profezzorn · 2025-08-17T04:38:39Z

styles/style_ptr.h

 // Get a pointer to class.
 template<class STYLE>
 StyleAllocator StylePtr() {
+  using _TopBaseT  = typename style_base_check_detail::TopBase<STYLE>::type;


Now that Layers<> can't become magically opaque anymore, we can remove this, right?
Edit:.. see below first

profezzorn · 2025-08-17T04:43:36Z

styles/style_ptr.h

+  using _TopBaseCol = decltype(std::declval<_TopBaseT&>().getColor(0));
+  static_assert(color_details::IsOpaqueColor<_TopBaseCol>::value,
+                "\n\n"
+                "*** StylePtr<> error: BASE LAYER MUST BE A SOLID COLOR, NOT TRANSPARENT.\n"


This might be confusing if the argument to styleptr is not a layer.
How about we move the actual assert into style_base_check_detail, and we'll make one error message if the argument is Layers<> with a transparent base, and another error message if it's something else that is transparent?

That should also give us a little bit more code reuse since this is currently repeated in StyleChargingPtr<>

another error message if it's something else that is transparent

hmm, meaning something like
AudioFlicker<AlphaL<Blue,Int<16000>>,AlphaL<Red,Int<16000>>> ?

How about we just leave the single message but remove "layer" from it so it just says "base must" - that covers a transparent style or a Layer.
"StylePtr<> error: BASE MUST BE A SOLID COLOR, NOT TRANSPARENT

I would use STYLE instead of BASE in that case.
(And I don't think we need the whole error to be allcaps.)

Since we still depend on finding the true base type buried inside Layers<>, TopBase seems to still be required. Otherwise a style of
StylePtr<Layers< AlphaL<Yellow,Int<9000>>, Red>>(), prints
"Layers<> error: Only the base color may be solid."
not the one it should, the
"StylePtr<> error: Style must be a solid color, not transparent." one.

After tonight's latest edits we have:

A transparent style with no layers like StylePtr<AudioFlicker<AlphaL<Blue,Int<16000>>,AlphaL<Red,Int<16000>>>>()
shows:
"*** StylePtr<> error: Style must be a solid color, not transparent."

A Layers<> style with a transparent first color like
StylePtr<Layers< AlphaL<Yellow,Int<9000>>, Red, AlphaL<Blue,Int<16000>>>>()
shows:
"*** StylePtr<> error: Style must be a solid color, not transparent."

A Layers<> style with an Alpha Zero first color gets a pass and compiles.

A Layers<> style with Black as the first color gets a pass and compiles.

A Layers<> style with Black and an Alpha Zero as the first 2 colors (in either order) gets a pass and compiles.

A Layers<> style with more than 2 solid colors anywhere shows:
"*** Layers<> error: Only the base color may be solid."

That's how it should be working, right?

I don't think we need the whole error to be all caps.

Are you sure I can't sell you on the great visibility the marquee style provides in the sea of errors?
Comparison:

ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:99:8: error: 'bool Style<T>::IsHandled(HandledFeature) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 99 | bool IsHandled(HandledFeature effect) override { | ^~~~~~~~~ ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:107:8: error: 'void Style<T>::run(BladeBase*) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 107 | void run(BladeBase* blade) override { | ^~~ ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:113:7: error: 'int Style<T>::get_max_arg(int) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 113 | int get_max_arg(int argument) override { | ^~~~~~~~~~~ In file included from ProffieOS-v8.x/ProffieOS/styles/fire.h:4, from ProffieOS-v8.x/ProffieOS/ProffieOS.ino:624: ProffieOS-v8.x/ProffieOS/styles/style_ptr.h: In instantiation of 'void style_base_check_detail::AssertLayersBaseOpaque() [with STYLE = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]': ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:160:57: required from 'StyleFactory* StylePtr() [with STYLE = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' ProffieOS-v8.x/ProffieOS/config/BC_Ronin_8.x.h:430:74: required from here ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:15:62: error: static assertion failed: *** StylePtr<> error: Style must be a solid color, not transparent. 15 | static_assert(color_details::IsOpaqueColor<_TopBaseCol>::value, | ^~~~~ In file included from ProffieOS-v8.x/ProffieOS/blades/blade_base.h:20, from ProffieOS-v8.x/ProffieOS/ProffieOS.ino:587: ProffieOS-v8.x/ProffieOS/styles/blade_style.h: In instantiation of 'BladeStyle* StyleFactoryImpl<STYLE>::make() [with STYLE = Style<Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> > >]': ProffieOS-v8.x/ProffieOS/styles/blade_style.h:45:15: required from here ProffieOS-v8.x/ProffieOS/styles/blade_style.h:47:22: error: cannot convert 'Style<Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> > >*' to 'BladeStyle*' in return 47 | return new STYLE(); | ^ exit status 1 Error compiling for board Proffieboard V2.

or

ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:99:8: error: 'bool Style<T>::IsHandled(HandledFeature) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 99 | bool IsHandled(HandledFeature effect) override { | ^~~~~~~~~ ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:107:8: error: 'void Style<T>::run(BladeBase*) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 107 | void run(BladeBase* blade) override { | ^~~ ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:113:7: error: 'int Style<T>::get_max_arg(int) [with T = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' marked 'override', but does not override 113 | int get_max_arg(int argument) override { | ^~~~~~~~~~~ In file included from ProffieOS-v8.x/ProffieOS/styles/fire.h:4, from ProffieOS-v8.x/ProffieOS/ProffieOS.ino:624: ProffieOS-v8.x/ProffieOS/styles/style_ptr.h: In instantiation of 'void style_base_check_detail::AssertLayersBaseOpaque() [with STYLE = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]': ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:160:57: required from 'StyleFactory* StylePtr() [with STYLE = Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> >]' ProffieOS-v8.x/ProffieOS/config/BC_Ronin_8.x.h:430:74: required from here ProffieOS-v8.x/ProffieOS/styles/style_ptr.h:15:62: error: static assertion failed: -------------------------------------------------------------------------- | | | StylePtr<> error: Style must be a solid color, not transparent | | | -------------------------------------------------------------------------- 15 | static_assert(color_details::IsOpaqueColor<_TopBaseCol>::value, | ^~~~~ In file included from ProffieOS-v8.x/ProffieOS/blades/blade_base.h:20, from ProffieOS-v8.x/ProffieOS/ProffieOS.ino:587: ProffieOS-v8.x/ProffieOS/styles/blade_style.h: In instantiation of 'BladeStyle* StyleFactoryImpl<STYLE>::make() [with STYLE = Style<Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> > >]': ProffieOS-v8.x/ProffieOS/styles/blade_style.h:45:15: required from here ProffieOS-v8.x/ProffieOS/styles/blade_style.h:47:22: error: cannot convert 'Style<Compose<AlphaL<Rgb<255, 255, 0>, SingleValueAdapter<IntSVF<5000> > >, Rgb<255, 0, 0> > >*' to 'BladeStyle*' in return 47 | return new STYLE(); | ^ exit status 1 Error compiling for board Proffieboard V2.

Not that my opinion matters, but I personally like the "framed" error much better than the one with "***".

profezzorn · 2025-08-17T04:54:04Z

styles/layers.h

-// If the base color is opqaque, the final result of this style will also be
-// opaque. If the base color is transparent, the final result may also be transparent,
-// depending on what the layers paint on top of the base color.
+// This style works like layers in Photoshop: each layer is painted over the one beneath it.


"beneath it" can be confusing, does it refer to the order of rending or the order of writing?
Not sure what a better way to describe it would be though...

Agreed. Unfortunately, it's true that "underneath" and "above" are equally correct for a way to describe layer order. I find myself thinking in terms of front to back, not top to bottom. So layers are (as it hints) like photoshop layers.
Therefore, the base layer is "behind" the other layers that are "in front of" it.

It can be made explicit with an example though:

Layers<A, B, C>

B is painted on top of A, and then C is painted on top of that.

I reworded a bit without the A, B, C example. Does it read well enough now?

profezzorn · 2025-08-18T07:46:56Z

styles/layers.h

 template<class BASE, class L1>
 class Compose {
 public:
+static_assert(!color_details::IsOpaqueColor<decltype(std::declval<L1&>().getColor(0))>::value,


Doesn't look fixed.
Note, I'm talking about the static_assert itself, not the string inside it.

profezzorn · 2025-08-18T07:52:24Z

styles/layers.h

+template<class BASE, class ABASE>
+struct LayerSelector<BASE, AlphaL<ABASE, Int<0>>> { typedef BASE type; };
+
+// Drop BLACK when it’s first (two-arg case).


This doesn't work, because the layer will be transparent.
Also, we already have a special case for BLACK in Compose<>

That's right.
I got lost in all these scenarios.

profezzorn · 2025-08-18T07:56:59Z

styles/style_ptr.h

+  template<class B, class L> struct TopBase<Compose<B,L>> : TopBase<B> {};
+  template<class STYLE>
+  inline void AssertLayersBaseOpaque() {
+    using _TopBaseT  = typename TopBase<STYLE>::type;


I don't think we need this, we can just check if STYLE is transparent.

I tried that, but it doesn’t correctly catch the case where a Layers<> starts with a transparent color because by the time the check runs, it already combined with the next layer, so we get the Layers error instead of the intended “Style must be solid color, not transparent” error.
With the TopBase check, we can directly see the very first color and report the right error message.

That would only happen if you have a transparent base and a solid layer somewhere after, right?
Isn't that a highly unusual case?

I would say yes. I was testing using
StylePtr<Layers< AlphaL<Yellow,Int<9000>>, Red, AlphaL<Blue,Int<16000>>>>()

But you've conditioned me to cover all the what-ifs.

When there are two errors in the code, I don't think we should be particularly picky about which one shows up.

profezzorn · 2025-08-23T23:46:35Z

styles/layers.h

-// If the base color is opqaque, the final result of this style will also be
-// opaque. If the base color is transparent, the final result may also be transparent,
-// depending on what the layers paint on top of the base color.
+// This style works like layers in Photoshop: each layer adds to the stack covering the base. 


Why replace "photoshop or gimp" with "photoshop"?
I don't know anybody who uses photoshop anymore, since gimp and krita are free and just as good in most cases.

I glazed right over that it even said Gimp.
Restored.

profezzorn · 2025-08-23T23:53:40Z

styles/layers.h

+// This style works like layers in Photoshop: each layer adds to the stack covering the base. 
+// The written order of layers in the blade style code puts the base first, with any following layers visually covering it.
+//
+// When a Layers<> is used as the first template argument to StylePtr<>, its BASE color (the first layer)


I'm not a fan of how this is written.
I think it would be better to explain that the output of Layers<> is opaque iff the base layer opaque.
Then we can explain that StylePtr<> it's argument to be opaque, so therefore StylePtr<Layers<BASE, ... >> requires BASE to be opaque.

The way it's written now, you can sort of infer what happens if the base layer is transparent, but it's never actually explained.

ok how about now?

profezzorn · 2025-08-23T23:53:54Z

styles/layers.h

+// When a Layers<> is used as the first template argument to StylePtr<>, its BASE color (the first layer)
+// must be a solid (opaque) color, not transparent. No additional solid colors may be stacked on top
+// of that solid base.
+// Additional layers stacked on top of a solid base color are transparent,


are -> must be

profezzorn · 2025-08-23T23:58:03Z

styles/style_ptr.h

+  template<class B, class L> struct TopBase<Compose<B,L>> : TopBase<B> {};
+  template<class STYLE>
+  inline void AssertLayersBaseOpaque() {
+    using _TopBaseT  = typename TopBase<STYLE>::type;


When there are two errors in the code, I don't think we should be particularly picky about which one shows up.

NoSloppy added 2 commits August 15, 2025 22:12

add error msgs

89017a8

add error msgs

00d33c5

profezzorn reviewed Aug 16, 2025

View reviewed changes

NoSloppy added 3 commits August 15, 2025 23:28

Move IsOpaqueColor trait into common/color.h

9f4dd41

Remove IsOpaqueColor from style_ptr.h and layers.h

22685b5

And make messages obnoxiously visible so they are easy to find in a sea of "ambiguous overload" errors.

Remove IsOpaqueColor from style_ptr.h and layers.h

f756531

And make messages obnoxiously visible so they are easy to find in a sea of "ambiguous overload" errors.

profezzorn reviewed Aug 16, 2025

View reviewed changes

NoSloppy added 4 commits August 16, 2025 02:23

simplify message printout

877ed74

insert the assert into Compose<>

8053cf8

also cover scenarios for Black and Alpha 0 being first

was bypassing Black Compose. removed specializations

a6939b3

Use opaque-layer rule in Compose<>

02be38b

profezzorn reviewed Aug 16, 2025

View reviewed changes

NoSloppy added 3 commits August 17, 2025 00:28

Edit LayerSelector, remove 3 arg case

60d2a68

edit error msg text

8e89135

actually remove 3-arg case for LayerSelector

b05e431

profezzorn reviewed Aug 17, 2025

View reviewed changes

NoSloppy added 3 commits August 18, 2025 00:17

try to clean up and make all work again

ee1b44e

move assert into style_base_check_detail

0f0f6d2

remove _BaseColorT

fa5fc51

profezzorn reviewed Aug 18, 2025

View reviewed changes

NoSloppy added 2 commits August 18, 2025 13:28

indention

2e64a61

remove redundant/incorrect Black specialization

848ce5e

profezzorn reviewed Aug 23, 2025

View reviewed changes

NoSloppy added 2 commits August 24, 2025 21:15

restore "Gimp" in description text.

c87e436

Update Usage

df0155c

Comments

Conversation

NoSloppy commented Aug 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoSloppy commented Aug 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoSloppy Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoSloppy commented Aug 16, 2025 •

edited

Loading

NoSloppy Aug 18, 2025 •

edited

Loading

NoSloppy Aug 19, 2025 •

edited

Loading