Screen::Pen: handle COLOR_RESET color values - #5866
Open
ChrisJohnsen wants to merge 3 commits into
Open
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.
ChrisJohnsen
force-pushed
the
cj/pen-COLOR_RESET
branch
from
August 5, 2026 13:47
646d875 to
dc37db3
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.