refactor: replace nearest_color duplicate with LbPaletteFindColour#4587
refactor: replace nearest_color duplicate with LbPaletteFindColour#4587cerwym wants to merge 1 commit intodkfans:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors palette nearest-colour lookup to use the shared LbPaletteFindColour() helper and updates the overlay lens effect to perform alpha blending in RGB space (instead of blending palette indices), then remapping back to the nearest palette entry.
Changes:
- Replaced
custom_sprites.c’s localnearest_color()withLbPaletteFindColour()when building the RGB→palette lookup table. - Fixed
OverlayEffectblending by unpacking palette RGB, blending channels, then mapping the blended RGB back to the nearest palette index viaLbPaletteFindColour(). - Added required
bflib_video.hincludes for access toLbPaletteFindColour()/ palette globals.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/kfx/lense/OverlayEffect.cpp | Switches overlay compositing to RGB-space alpha blend + palette remap using LbPaletteFindColour(). |
| src/custom_sprites.c | Removes duplicated nearest-colour implementation and uses LbPaletteFindColour() for RGB→palette table generation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (uint32_t i = 0; i < table_size; ++i) { | ||
| rgb_to_pal_table[i] = nearest_color(i, palette); | ||
| rgb_to_pal_table[i] = LbPaletteFindColour(palette, | ||
| i & (MAX_COLOR_VALUE - 1), | ||
| (i >> 6) & (MAX_COLOR_VALUE - 1), | ||
| (i >> 12) & (MAX_COLOR_VALUE - 1)); |
There was a problem hiding this comment.
This replacement changes the nearest-colour metric: the removed nearest_color() used a simple sum of absolute channel deltas, while LbPaletteFindColour() uses squared-distance plus multiple tie-break passes. If the intent is to keep sprite conversion identical, this will produce different palette indices for some RGB inputs; consider either keeping the prior distance metric (e.g., via a dedicated helper) or updating expectations/docs to reflect the intentional behavior change.
There was a problem hiding this comment.
Need to see the difference on this
| for (uint32_t i = 0; i < table_size; ++i) { | ||
| rgb_to_pal_table[i] = nearest_color(i, palette); | ||
| rgb_to_pal_table[i] = LbPaletteFindColour(palette, | ||
| i & (MAX_COLOR_VALUE - 1), | ||
| (i >> 6) & (MAX_COLOR_VALUE - 1), | ||
| (i >> 12) & (MAX_COLOR_VALUE - 1)); | ||
| } |
There was a problem hiding this comment.
load_rgb_to_pal_table() calls LbPaletteFindColour() 64^3 times; because LbPaletteFindColour() does multiple 256-entry passes, this may significantly increase one-time startup/mod load time compared to the prior single-pass implementation. If this becomes noticeable, consider caching/precomputing the table to disk or using a single-pass search tailored to the 6-bit palette format for this bulk build step.
Remove the local nearest_color() from custom_sprites.c — LbPaletteFindColour() in bflib_video already provides the same palette-search. Using the existing function eliminates duplication across three sites. Also fix OverlayEffect RGB alpha blending: was incorrectly blending palette indices as integers; now pre-builds a 256x256 blend LUT per render call (65280 palette lookups once) instead of calling LbPaletteFindColour per pixel.
cerwym
force-pushed
the
fix/palette-nearest-colour
branch
from
14a5285 to
f6b9ecc
Compare
|
Leaving this open to perf issues resolved |
I assume you meant to Convert to draft then |
|
Tomato potato |
Remove the local nearest_color() from custom_sprites.c — LbPaletteFindColour() in bflib_video already provides the same palette-search. Using the existing function eliminates duplication across three sites.
Also fix OverlayEffect RGB alpha blending: was incorrectly blending palette indices as integers; now unpacks RGB from palette, blends in colour space, then maps back via LbPaletteFindColour.