fix(wow-blp): palettized BLP2 colour map is BGRA, not RGBA — swap R↔B#58
Closed
samwhosung wants to merge 2 commits into
Closed
fix(wow-blp): palettized BLP2 colour map is BGRA, not RGBA — swap R↔B#58samwhosung wants to merge 2 commits into
samwhosung wants to merge 2 commits into
Conversation
…ture-anim chunks Some Classic (v256) models (e.g. GeneralTorch01, WestfallLamppost02, Candle02, Kobold, Imp, ElwynnCampfire) fail parse_m2 with "I/O error: failed to fill whole buffer". The particle-emitter section (and, for some models, lights / texture-animations) reads past EOF. These chunks are cosmetic — a malformed read in one shouldn't abort parsing of the model's geometry. Make those three reads and their keyframe collects degrade to an empty list (with a log warning) instead of propagating the error. Geometry, skin, textures, materials, bones, etc. remain strict. This lets consumers that only need geometry load these models. Fixes wowemulation-dev#56. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The BLP2 colour map is stored BGRA per the format spec: byte 0 = B, byte 1 = G, byte 2 = R, byte 3 = A. Both the decoder (`convert/raw1.rs`) and the encoder (`convert/palette.rs`) treated the palette as RGBA, so every real Blizzard palettized atlas decoded with R and B swapped. Concretely visible on vanilla 1.12.1 `World\NoDXT\Detail\WestfallDetailDoodads01.blp` (compression=1, alpha_size=1): the authored brown/tan grass and yellow wheat palette (avg R=167 G=147 B=78; 201 of 253 non-zero entries red-dominant) rendered as saturated cool blue/teal (avg R=78 G=147 B=167) — and the visible bug presented as "blue Westfall clutter" in any downstream client. Other vanilla zones (Elwynn, Duskwood, Stranglethorn, …) use DXT-compressed BLPs which take a different code path that handles channel order correctly via squish, hiding the bug everywhere except the few palettized atlases. Round-trip tests passed both before and after the fix because the encoder + decoder are equally consistent at either swap polarity; only reading real spec-conforming Blizzard BLPs exposes the bug. Co-Authored-By: Sam <samuv.eth@gmail.com>
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Member
|
I merged this together with an update to the actual comments referencing this to make it a bit more complete. Thanks very much for the submission. |
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
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.
Summary
The BLP2 colour-map for palettized textures (
compression = 1) is stored BGRA per the format spec (byte 0 = B, byte 1 = G, byte 2 = R, byte 3 = A). Both the decoder inconvert/raw1.rs(all fouralpha_bitsbranches) and the encoder inconvert/palette.rstreated the palette as RGBA, so every real Blizzard palettized atlas decoded with R and B channels swapped.Round-trip tests pass at either swap polarity because the encoder + decoder are equally consistent with themselves — only reading spec-conforming Blizzard data exposes the bug. This is why it has lived in the crate's regression suite undetected.
Reproducer
Read
World\NoDXT\Detail\WestfallDetailDoodads01.blpfrom a vanilla 1.12.1 (build 5875)texture.MPQ(BLP2 v1,compression=1,alpha_size=1,alpha_type=8, 256×256). The authored palette (256 BGRA DWORDs at offset 148) is wheat-field tan: averageR=167 G=147 B=78, with 201 of 253 non-zero entries red-dominant and the most-saturated entries being yellows and oranges like(253, 246, 0)and(249, 215, 3).Decoded via
wow-blp0.6.4, the output is saturated cool blue/teal: average(78, 147, 167)— the same palette with R and B swapped.Downstream impact
Discovered while chasing a "Westfall clutter is blue" visual bug in a from-scratch 1.12.1 client (
mindforge-client). After patching the crate via[patch.crates-io], the bug fully resolves. Across a full vanilla 1.12.1./WoW/DataMPQ chain, the palettized code path is hit by:Shared clutter atlases (
World\NoDXT\Detail\…) — 4 files affected:WestfallDetailDoodads01.blp(Westfall)DeadwindDetails01.blp(Deadwind Pass)ArathiHighlandsDetails.blp(Arathi Highlands)EasternPlaguelandsDetails_256.blp(Eastern Plaguelands)The other 33 shared clutter atlases (Elwynn, Duskwood, Stranglethorn, Tirisfal, Barrens, Durotar, Mulgore, Burning Steppes, Felwood, Hyjal, Un'Goro, Tanaris, Silithus, Loch Modan, Redridge, Silverpine, …) ship as DXT (
compression=2), which takes the separatedxtn.rscode path that handles channel order viasquish. Hence "fine everywhere except Westfall" as a visible diagnosis.Terrain
TILESET\…\*.blptextures — 13 of 1050 affected:Tileset\EmeraldDream\…(Emerald Dream — not a player-accessible map in 1.12)Tileset\RedRidge\…(RedridgeRockBase_s.blp,RedridgeRockRoadBase_s.blp— both_sspecular auxiliaries, not the diffuse colour)The remaining 1037 terrain BLPs are DXT and were unaffected — the visible terrain colour of every accessible zone was correct before and after.
The fix
Decoder (
convert/raw1.rs, applied in all fouralpha_bitsbranches):Encoder (
convert/palette.rs) fixed in matching direction so the crate's internal round-trip stays consistent. All existing tests pass.A note on regression testing
This bug demonstrates a known weakness of round-trip-only regression tests for serialisation formats: the encoder + decoder were equally consistent at the wrong polarity, so the round-trip never caught it. A useful follow-up (out of scope for this PR) would be adding a small golden-vector test against a checked-in palette + indices fixture with hand-verified RGB expected output — that would catch the kind of "two-wrongs round-trip green" failure mode this PR fixes.
Closes / refs
Not aware of an existing issue tracking this; happy to open one and link if preferred.