fix: implement comprehensive CGB sprite priority system #45
Conversation
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
This commit fixes two critical PPU rendering bugs identified in cgb-acid2 test: 1. **8×16 Sprite Vertical Flip Bug** - Previously: flipped line number BEFORE selecting which tile (top/bottom) - This incorrectly swapped tiles when Y-flip was enabled - Now: select tile based on unflipped position, then flip within that tile - Fixes mouth rendering in cgb-acid2 (1,306 pixel differences) 2. **CGB Master Priority Bug** - Previously: sprite priority flag always respected in CGB mode - Missing: when LCDC bit 0 is clear, BG loses priority in CGB mode - Now: sprites always render on top when master priority is disabled - Fixes nose rendering in cgb-acid2 (238 pixel differences) According to Pan Docs, for 8×16 sprites with Y-flip: - Tiles stay in same physical position (top tile at top, bottom at bottom) - Each 8×8 tile is flipped individually within itself - Line selection must happen BEFORE applying flip For CGB Master Priority (LCDC bit 0): - When clear, BG/Window lose priority regardless of sprite flags - Sprites always appear on top in this mode - Used by cgb-acid2 test for nose diamond shape All PPU unit tests passing (56 tests, 23183 assertions).
Updated test result showing improvements from 8×16 sprite Y-flip and CGB master priority fixes: - Mouth now renders as horizontal black line (8×16 sprites fixed) - Nose renders more correctly (master priority improved) - Current similarity: 81.62% Remaining issues (4,234 pixels): - 2,864 pixels: Yellow face → white (palette/priority issue) - 512 pixels: Black outlines → white (palette/priority issue) - 768 pixels: Various color swaps These fixes address the tile flipping and priority logic, but there appear to be additional palette selection or rendering order issues that need investigation.
This commit fixes the priority handling system based on cgb-acid2 README:
1. **Reverted 8×16 Sprite Y-flip** (my previous fix was incorrect)
- Original code was correct: Y-flip DOES swap tiles for 8×16 sprites
- With Y-flip: line 0 reads from bottom tile line 7, line 15 reads from top tile line 0
- Mouth now renders correctly as horizontal black line ✓
2. **Implemented Complete CGB Priority System**
- Added bgPriorityBuffer to store BG-to-OAM Priority flag (bit 7 of BG attributes)
- Proper priority logic:
* Master Priority disabled (LCDC bit 0 = 0) → sprites always on top
* Master Priority enabled (LCDC bit 0 = 1):
- BG-to-OAM Priority set → BG on top
- OBJ-to-BG Priority set → BG on top
- Otherwise → sprite on top
- Nose now renders correctly as diamond ✓
- Eyes render correctly with green centers ✓
**Test Results:**
- Similarity: 85.35% (improved from 81.62%)
- Matching pixels: 19,664 / 23,040
- Remaining issues:
* 2,864 pixels: Yellow face → white
* 512 pixels: Black outline → white
The priority system now correctly implements both Master Priority and
BG-to-OAM Priority as described in cgb-acid2 README sections for
nose (Master Priority test) and eyes (BG-to-OAM Priority test).
All PPU unit tests passing (56 tests, 23183 assertions).
3a98090 to
a821fa3
Compare
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.
This PR implements a complete CGB sprite priority system to fix rendering issues in the cgb-acid2 test.
Changes
1. BG Priority Buffer
bgPriorityBufferto track BG-to-OAM Priority flag (bit 7 of BG attributes)bgColorBufferfor accurate priority decisions2. CGB Priority System
Implemented comprehensive three-tier priority logic for CGB mode:
Master Priority (LCDC bit 0)
BG-to-OAM Priority (BG attribute bit 7)
bgPriorityBufferOBJ-to-BG Priority (sprite attribute bit 7)
3. 8×16 Sprite Handling
4. DMG Mode Compatibility
Test Results