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.
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user