Screen::Pen: handle COLOR_RESET color values - #5866
Conversation
`COLOR_RESET` seems like it should not be applied to drawing into DF buffers. Callers should probably specify actual colors. For Pen colors coming from Lua: - intercept `COLOR_RESET` values and translate them to `COLOR_GREY` (fg) or `COLOR_BLACK` (bg), and - mask off all values to four bits to keep them in the 16 color range. Those grey and black "default" colors are the default color values provided by the Pen constructor. They also correspond to the colors used when `fg` and `bg` are nil coming from Lua. On the C++ side, the constructors and color-modifying member functions now mask off the color values to four bits, but do not do the fancier fg/bg-based defaulting. Since `Screen::Pen` is a struct with public data members, there is no good place to fully intercept changes to the color values. Add unit tests for the `fg`/`bg`/`tile_fg`/`tile_bg` handling of the Lua `dfhack.pen` interface to `Screen::Pen`. Most of the tests (those not in the last `COLOR_RESET` "section") document existing functionality and pass without the changes in this commit. Masking is not currently tested. Convert some "magic numbers" being used for Pen colors to their `color_value` enumerator names.
646d875 to
dc37db3
Compare
|
I've tried a few more variations locally:
The traceback from raising an error can be helpful in tracking down the source of the erroneous color, but actually raising an error can be pretty intrusive (due to minimal Capturing a trackback without raising an actual error provides most of the benefit of raising an error (except the big, obvious render breakage, which is of possibly dubious value…). Masking the value might not be terribly useful since there is now code to check for out-of-bounds colors before they are actually used (#5864). I'm leaning towards "warn with traceback" (no masking) and letting |
1 participant
Here is some mitigation code that I put together for #5865.
It largely focuses on the Lua-originated color value handling. Perfect color value protection seems much less tractable on the C++ side since the relevant data members are all public.
Note: this will prevent the logging in #5864 from triggering for Lua-originated sources of out-of-range color values. Should similar logging be done in the places where this change does defaulting and masking?
If COLOR_RESET handling and automatic masking is thought to be good idea, it might be worth un-static-ing the function that does the "reset handling and masking" and use it in the few places (ctors, etc.) that the "C++ code" in this PR does simple masking. Overall there seems to be much less drawing code in C++, and what I saw (from asking clangd for Pen ctor uses) seemed okay (all using "proper" color values).
The color "masking" behavior is not covered in the unit tests, since I was not sure if it was the overall right direction.