hex triplets for colors

kiln-core/src/colors.rs

@@ -32,20 +32,5 @@ pub const NAMED_COLORS: [(&str, Rgba8); 16] = [
     ("White", rgb(0xFF, 0xFF, 0xFF)),
 ];
 
-/// Parses an `"#RRGGBB"` hex color string into an [`Rgba8`].
-/// Returns opaque black on any parse failure.
-pub(crate) fn parse_color(hex: &str) -> Rgba8 {
-    let hex = hex.trim_start_matches('#');
-    if hex.len() != 6 {
-        return Rgba8 {
-            r: 0,
-            g: 0,
-            b: 0,
-            a: 255,
-        };
-    }
-    let r = u8::from_str_radix(&hex[0..2], 16).unwrap_or(0);
-    let g = u8::from_str_radix(&hex[2..4], 16).unwrap_or(0);
-    let b = u8::from_str_radix(&hex[4..6], 16).unwrap_or(0);
-    Rgba8 { r, g, b, a: 255 }
-}
+// Hex color parsing/formatting lives in `glyph.rs` alongside the `Glyph` serde
+// adapter that uses it — see `glyph::parse_color` / `glyph::color_to_hex`.

kiln-core/src/glyph.rs

@@ -22,8 +22,14 @@ pub struct Glyph {
     /// Which tile to draw
     pub tile: u32,
     /// Foreground color, applied to non-background pixels of the tile.
+    ///
+    /// Serialized as an `"#RRGGBB"` hex string — see [`parse_color`].
+    #[serde(with = "hex_color")]
     pub fg: Rgba8,
     /// Background color, drawn as a filled rectangle behind the tile.
+    ///
+    /// Serialized as an `"#RRGGBB"` hex string — see [`parse_color`].
+    #[serde(with = "hex_color")]
     pub bg: Rgba8,
 }
 
@@ -93,43 +99,149 @@ impl Glyph {
     }
 }
 
-// TODO make TileIndex work again
-/*
-/// A tile index in a palette entry: either a plain integer or a character literal.
+// TODO make TileIndex work again: `tile` should accept either a single-character
+// string (used directly as the display char) or a `u8` (looked up in CP437). That
+// change turns `Glyph::tile` into a `char`, with `'\0'` as the transparent
+// sentinel and `CP437[0]` remapped to `'\0'` to match.
+
+/// Parses an `"#RRGGBB"` hex color into an opaque [`Rgba8`].
 ///
-/// Accepting both forms lets map files write `tile = 35` or `tile = "#"` (the char
-/// is converted to its Unicode scalar) interchangeably.
-#[derive(Deserialize, Serialize, Eq, Clone, Copy, Debug)]
-#[serde(untagged)]
-pub enum TileIndex {
-    /// A direct tile index (e.g. `tile = 35`).
-    Num(u32),
-    /// A single-character shorthand (e.g. `tile = "#"`); converted to its Unicode scalar.
-    Chr(char),
+/// The leading `#` is optional. Returns a human-readable `Err` describing the
+/// problem — map loading surfaces it with the file position, and the script color
+/// API logs it.
+///
+/// **Alpha is deliberately not part of the format.** A cell draws exactly one
+/// glyph, chosen by the fixed precedence in
+/// [`Board::glyph_at`](crate::board::Board::glyph_at) — there is no compositing
+/// pass, so there is nothing for a translucent color to blend against. (A
+/// see-through cell is expressed by tile `0`, which falls through to whatever the
+/// next precedence level draws; that is a different mechanism entirely.) Every
+/// front-end drops alpha at the render boundary, so accepting an `"#RRGGBBAA"`
+/// here would silently do nothing — an `Err` is more honest.
+pub fn parse_color(hex: &str) -> Result<Rgba8, String> {
+    let digits = hex.strip_prefix('#').unwrap_or(hex);
+    if digits.len() != 6 {
+        return Err(format!(
+            "expected a hex color like \"#RRGGBB\", got {hex:?}"
+        ));
+    }
+    // Two hex digits at `offset`, or an error naming the offending channel.
+    let byte = |offset: usize, channel: &str| -> Result<u8, String> {
+        u8::from_str_radix(&digits[offset..offset + 2], 16)
+            .map_err(|_| format!("{channel} channel of {hex:?} is not a hex byte"))
+    };
+    Ok(Rgba8 {
+        r: byte(0, "red")?,
+        g: byte(2, "green")?,
+        b: byte(4, "blue")?,
+        a: 255,
+    })
 }
 
-impl TileIndex {
-    /// Returns the tile index as a `u32`, converting a char to its scalar value.
-    pub(crate) fn into_u32(self) -> u32 {
-        match self {
-            TileIndex::Num(n) => n,
-            TileIndex::Chr(c) => c as u32,
-        }
+/// Formats an [`Rgba8`] as `"#RRGGBB"`. The inverse of [`parse_color`].
+///
+/// Alpha is not emitted — see [`parse_color`] for why it is not part of the
+/// format. Colors in a board are always opaque, so nothing is lost.
+pub fn color_to_hex(c: Rgba8) -> String {
+    format!("#{:02X}{:02X}{:02X}", c.r, c.g, c.b)
+}
+
+/// `serde` adapter letting [`Glyph`]'s color fields round-trip as hex strings.
+///
+/// `Rgba8` comes from the `color` crate, so it cannot implement `Serialize` /
+/// `Deserialize` for our format directly; `#[serde(with = "hex_color")]` on each
+/// field routes through [`parse_color`] / [`color_to_hex`] instead of the
+/// structural `{ r, g, b, a }` form.
+mod hex_color {
+    use super::{color_to_hex, parse_color};
+    use color::Rgba8;
+    use serde::{Deserialize, Deserializer, Serializer};
+
+    pub fn serialize<S: Serializer>(color: &Rgba8, s: S) -> Result<S::Ok, S::Error> {
+        s.serialize_str(&color_to_hex(*color))
+    }
+
+    pub fn deserialize<'de, D: Deserializer<'de>>(d: D) -> Result<Rgba8, D::Error> {
+        let text = String::deserialize(d)?;
+        parse_color(&text).map_err(serde::de::Error::custom)
     }
 }
 
-impl PartialEq for TileIndex {
-    fn eq(&self, other: &Self) -> bool {
-        self.into_u32() == other.into_u32()
-    }
-}
+#[cfg(test)]
+mod tests {
+    use super::{color_to_hex, parse_color, Glyph};
+    use color::Rgba8;
 
-impl Into<u32> for TileIndex {
-    fn into(self) -> u32 {
-        match self {
-            TileIndex::Num(n) => n,
-            TileIndex::Chr(c) => c as u32,
-        }
+    #[test]
+    fn parse_color_accepts_six_digits_with_optional_hash() {
+        let expected = Rgba8 { r: 0x11, g: 0x22, b: 0x33, a: 255 };
+        assert_eq!(parse_color("#112233").unwrap(), expected);
+        assert_eq!(parse_color("112233").unwrap(), expected, "leading # is optional");
+        assert_eq!(parse_color("#AABBCC").unwrap(), parse_color("#aabbcc").unwrap());
     }
-}
- */
+
+    #[test]
+    fn parse_color_rejects_bad_input() {
+        // Wrong length, non-hex digits, and empty all report rather than
+        // silently yielding black (which would be an invisible authoring bug).
+        assert!(parse_color("#12345").is_err());
+        assert!(parse_color("#1122334").is_err());
+        assert!(parse_color("#gg2233").is_err());
+        assert!(parse_color("").is_err());
+    }
+
+    #[test]
+    fn parse_color_rejects_an_alpha_channel() {
+        // A cell draws one glyph by precedence with no compositing pass, and every
+        // front-end drops alpha at the render boundary — so "#RRGGBBAA" would be
+        // silently ignored. Rejecting it tells the author instead.
+        assert!(parse_color("#11223380").is_err());
+    }
+
+    #[test]
+    fn color_to_hex_never_emits_alpha() {
+        assert_eq!(color_to_hex(Rgba8 { r: 1, g: 2, b: 3, a: 255 }), "#010203");
+        // Even a non-opaque value (which a board never holds) stays six digits,
+        // so output always round-trips back through parse_color.
+        assert_eq!(color_to_hex(Rgba8 { r: 1, g: 2, b: 3, a: 4 }), "#010203");
+    }
+
+    #[test]
+    fn glyph_colors_round_trip_through_toml_as_hex() {
+        let glyph = Glyph {
+            tile: 35,
+            fg: Rgba8 { r: 0x80, g: 0x80, b: 0x80, a: 255 },
+            bg: Rgba8 { r: 0x60, g: 0x60, b: 0x60, a: 255 },
+        };
+        let text = toml::to_string(&glyph).unwrap();
+        assert!(text.contains(r##"fg = "#808080""##), "fg emitted as hex: {text}");
+        assert_eq!(toml::from_str::<Glyph>(&text).unwrap(), glyph);
+    }
+
+    #[test]
+    fn glyph_deserializes_from_hand_written_hex() {
+        let glyph: Glyph = toml::from_str(
+            r##"
+            tile = 35
+            fg = "#808080"
+            bg = "#606060"
+        "##,
+        )
+        .unwrap();
+        assert_eq!(glyph.fg, Rgba8 { r: 0x80, g: 0x80, b: 0x80, a: 255 });
+        assert_eq!(glyph.bg, Rgba8 { r: 0x60, g: 0x60, b: 0x60, a: 255 });
+    }
+
+    #[test]
+    fn glyph_reports_a_malformed_color_instead_of_defaulting() {
+        let err = toml::from_str::<Glyph>(
+            r##"
+            tile = 35
+            fg = "#nothex"
+            bg = "#606060"
+        "##,
+        )
+        .unwrap_err();
+        assert!(err.to_string().contains("hex"), "error mentions hex: {err}");
+    }
+}

kiln-core/src/script.rs

@@ -49,7 +49,8 @@ use crate::api::player::PlayerWithPos;
 use crate::api::queue::ObjQueue;
 use crate::api::registry::Registry;
 use crate::builtin::BUILTIN_SOURCES;
-use crate::colors::parse_color;
+use color::Rgba8;
+use crate::glyph::parse_color;
 use crate::glyph::Glyph;
 use crate::keys::Keyring;
 use crate::player::PlayerRef;
@@ -506,8 +507,22 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
         }
     );
 
+    // Parses a script-supplied "#RRGGBB" color, logging and dropping a malformed
+    // one (a `None` channel leaves that color unchanged) rather than silently
+    // painting it black, which is invisible to debug.
+    fn color_arg(text: &str, fn_name: &str, log_sink: &LogSink) -> Option<Rgba8> {
+        match parse_color(text) {
+            Ok(color) => Some(color),
+            Err(err) => {
+                log_sink.error(format!("{fn_name}: {err}"));
+                None
+            }
+        }
+    }
+
     // set_fg(fg): change foreground color only.
     let b = board.clone();
+    let sink = log_sink.clone();
     engine.register_fn(
         "set_fg",
         move |ctx: NativeCallContext, fg: ImmutableString| {
@@ -515,7 +530,7 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
                 &b,
                 source_of(&ctx),
                 Action::SetColor {
-                    fg: Some(parse_color(fg.as_str())),
+                    fg: color_arg(fg.as_str(), "set_fg", &sink),
                     bg: None,
                 },
             );
@@ -524,6 +539,7 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
 
     // set_bg(bg): change background color only.
     let b = board.clone();
+    let sink = log_sink.clone();
     engine.register_fn(
         "set_bg",
         move |ctx: NativeCallContext, bg: ImmutableString| {
@@ -532,7 +548,7 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
                 source_of(&ctx),
                 Action::SetColor {
                     fg: None,
-                    bg: Some(parse_color(bg.as_str())),
+                    bg: color_arg(bg.as_str(), "set_bg", &sink),
                 },
             );
         },
@@ -540,6 +556,7 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
 
     // set_color(fg, bg): change both colors.
     let b = board.clone();
+    let sink = log_sink.clone();
     engine.register_fn(
         "set_color",
         move |ctx: NativeCallContext, fg: ImmutableString, bg: ImmutableString| {
@@ -547,8 +564,8 @@ fn register_write_api(engine: &mut Engine, board: BoardRef, log_sink: LogSink) {
                 &b,
                 source_of(&ctx),
                 Action::SetColor {
-                    fg: Some(parse_color(fg.as_str())),
-                    bg: Some(parse_color(bg.as_str())),
+                    fg: color_arg(fg.as_str(), "set_color", &sink),
+                    bg: color_arg(bg.as_str(), "set_color", &sink),
                 },
             );
         },