fix(md10): md10 shadow crash and restore sprite texture bounds#4547
fix(md10): md10 shadow crash and restore sprite texture bounds#4547Loobinex merged 1 commit intodkfans:masterfrom
Conversation
Added descriptive headers for functions I know about Added inline comments for explanation of md10 to best of my understanding [md10 crash investigation] guards added to capture root cause if crash recurs
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent/diagnose an md10 (creature shadow) rendering crash and correct sprite-shadow texture addressing, while also documenting several trig render modes.
Changes:
- Add guardrails/logging for invalid/NULL keepersprite lookups and suspicious shadow dimensions.
- Fix md10 shadow texture addressing to use full 256×256 V-range (not 32-row masking) and adjust sampling/blending logic for shadow rendering.
- Add descriptive function header comments and inline explanations in the trig renderer.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/engine_render.c | Adds validation/logging around computed shadow sprite dimensions before enqueueing shadow geometry. |
| src/creature_graphics.c | Adds NULL-check + logging when sprite lookup fails in get_keepsprite_unscaled_dimensions(). |
| src/bflib_render_trig.c | Documents trig render modes and updates md10 to index 256×256 textures and blend shadows via fade tables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| struct KeeperSprite* kspr = sprite_by_frame(kspr_anim); | ||
| if (kspr == NULL) | ||
| { | ||
| ERRORLOG("[md10 crash investigation] NULL sprite returned for anim=%ld angle=%ld frame=%ld", kspr_anim, angle, frame); | ||
| *orig_w = 0; | ||
| *orig_h = 0; | ||
| *unsc_w = 0; | ||
| *unsc_h = 0; | ||
| return; |
There was a problem hiding this comment.
get_keepsprite_unscaled_dimensions() now guards against kspr == NULL, but it still does pointer arithmetic (kspr += frame / rotated frame offset) later without validating that frame is within the animation's FramesCount. If frame is out of range this will read past the sprite table (undefined behavior / potential crash). Consider validating frame (and the computed rotated-frame index for Rotable == 2) up front; on failure, log and return zeros similar to the NULL-sprite path.
| } | ||
| { |
There was a problem hiding this comment.
spr from keepersprite_array(thing->anim_sprite) is dereferenced later (spr->shadow_offset), but there is no explicit NULL-check. The new dimension guard will return early for some invalid sprite cases, but a direct if (spr == NULL) return; before using it would make this path robust and avoid relying on downstream dimension side-effects.
| } | |
| { | |
| } | |
| if (spr == NULL) | |
| { | |
| WARNLOG("[md10 crash investigation] NULL sprite for thing %d (anim=%d frame=%d)", | |
| thing->index, thing->anim_sprite, thing->current_frame); | |
| return; | |
| } | |
| { |
| // Sample texture; if non-zero (inside shadow), apply fade | ||
| if (m[colM]) { | ||
| // Fade table lookup: high byte = shadow intensity, low byte = screen pixel | ||
| // Result is blended shadow color | ||
| colS = (vec_colour << 8) | (*o); | ||
| *o = f[colS]; | ||
| } |
There was a problem hiding this comment.
Brace placement is inconsistent with the surrounding style in this file: most conditionals put { on the next line, but this block uses if (m[colM]) {. Consider matching the existing brace style for readability/consistency.
2 participants
Added descriptive headers for functions I know about Added inline comments for explanation of md10 to best of my understanding [md10 crash investigation] guards added to capture root cause if crash recurs