From 82d7c4f8a78e71bd7c4b5d58d6dfaf4a6cec62a7 Mon Sep 17 00:00:00 2001 From: ReaperOfVeriod Date: Wed, 14 Oct 2026 04:36:25 +0200 Subject: [PATCH] Three implementations of the 0x80 alpha rule; the one decoding pixels was wrong Found the PS2 alpha convention expressed three times in three files, and the three disagree: value texture::scale_alpha gs::alpha_to_8 gc-engine (float) 0x40 128 128 0.5 0x7E 252 251 250.9 0x7F 254 253 253.0 0x80 255 255 1.0 scale_alpha was `if a < 0x80 { a * 2 } else { 0xff }`. That is a/128 * 256, not a/128 * 255 -- one too high at the top of the range, so 0x7F decoded to 254 where the renderer's own alpha path gives 253. It is the only one of the three that was wrong, and it was the one feeding decoded texture pixels. scale_alpha now delegates to gs::alpha_to_8. The renderer keeps its float form because it wants a float, and gc-formats has no business importing an engine concern; two of the three are now one function. The visual cost of this bug was at most 1/255 on a single palette entry. Nothing would ever have shown it in a screenshot. That is the point rather than the excuse: this class of error is not "big wrong", it is "right to within a rounding step", in the code path nobody instruments. Every one of them is invisible by construction, which is why the only defence is noticing that one rule has three homes and making it have one. Worth counting what this has turned into over three commits. The vtbl terminator: two loops reading the same table with opposite answers. The sampler: a comment asserting CLAMP over code doing Repeat. The alpha rule: three implementations, one wrong. None of these is a subtle algorithm and none is in an exotic format. All three are the same defect -- a quantity that should have one definition had several, and nothing in the build made them compare. --- crates/gc-formats/src/texture.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/crates/gc-formats/src/texture.rs b/crates/gc-formats/src/texture.rs index a6f4240..a8c45c8 100644 --- a/crates/gc-formats/src/texture.rs +++ b/crates/gc-formats/src/texture.rs @@ -18,6 +18,7 @@ //! anything above it clamps to opaque. use crate::buf::{Error, ErrorKind, Result}; +use crate::gs; use crate::level_core::TextureEntry; /// PS2 CSM1 CLUT order to linear: within each 32-entry group, swap the two @@ -30,13 +31,20 @@ pub fn clut_index(i: u32) -> u32 { } } -/// PS2 alpha 0..=0x80 to 0..=0xff. `0x80` is opaque; above that clamps. +/// PS2 alpha `0x00..=0x80` to `0x00..=0xff`. `0x80` is opaque; above clamps. +/// +/// Delegates to [`crate::gs::alpha_to_8`] rather than computing here. This used +/// to be `if a < 0x80 { a * 2 } else { 0xff }`, which is `a / 128 * 256` -- one +/// too high at the top of the range, so `0x7F` decoded to 254 where the renderer's +/// own alpha path and gs::alpha_to_8 both give 253. Three expressions of one rule, +/// two of them right, and the wrong one was the one decoding actual pixels. +/// +/// The visual difference is at most 1/255 on a single palette entry and nothing +/// anyone would ever see. That is exactly why it survived: the bug class here is +/// not "big wrong", it is "right to within a rounding step", in the code path +/// that nobody instruments. pub fn scale_alpha(a: u8) -> u8 { - if a < 0x80 { - a * 2 - } else { - 0xff - } + gs::alpha_to_8(a) } /// A decoded RGBA8 image.