brainray: add rl_draw_texture_rec - #293
Conversation
rl_draw_texture blits a whole image, so an animation meant one file per frame and a sprite could not face the other direction. DrawTextureRec takes a source rectangle, which is what makes atlases, animation strips and tiled parallax possible from Brainrot at all. Two aggregates come apart here rather than one: raylib's `rec` is a Rectangle of four floats and `position` is a Vector2 of two, so the source box is sx, sy, sw, sh and the destination corner is x, y. The float position differs from rl_draw_texture's int x, y and that mirrors raylib rather than contradicting it -- DrawTexture takes ints, DrawTextureRec takes a Vector2 -- and sub-pixel placement is what a scrolling background wants anyway. A negative sw mirrors horizontally and a negative sh vertically, raylib's own idiom. Until DrawTexturePro's rotation is exposed that is the only way to flip a sprite, so it is documented rather than left to look like a bug. Verified by eye against a four-frame atlas: sub-rect slicing, both mirror directions, a half-frame slice and a tint all render correctly. On what the new test does and does not establish, since the last review was right to press on this. It verifies the load contract -- a generated PNG gives a non-negative handle, a missing file gives -1, which had no test before and fails if flipped to 0 -- and that the whole sequence exits clean under ASan with no leak. It does not verify which pixels landed, because brainray cannot read a texture or the framebuffer back; an implementation ignoring `rec` would pass. It also does not verify the handle guards: SO_CFLAGS is `-fPIC -shared` with no sanitizers, as for every shared object here including libstdrot.so, so an out-of-bounds read of g_textures[] inside the module is invisible to ASan. Deleting the bounds check still passes -- I tried it. The docstring says all of this instead of implying coverage the test does not have. Closing either gap needs something outside this PR: a pixel-readback wrapper for the first, sanitizer-instrumented shared objects for the second. The test writes its own 8x4 PNG from zlib and struct, so the fixture adds no imaging dependency and nothing is checked in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Stale comment
APPROVE
This is a flatten of
DrawTextureReconto the existing Road A scalar ABI. I traced declaration →STDROT_EXPORT_SIG→coerce_numeric→.val.f/.val.ireads →Rectangle/Vector2/make_color(args, 7)→BR_RAYLIB_VOID(DrawTextureRec(...)). Those layers agree.Handle validation is the same three-clause guard as
rl_draw_texture/rl_unload_texture. Negativesw/shis raylib's actual flip idiom, not folklore:DrawTextureRecsizes the dest withfabsfof the source, andDrawTextureProflips on source-sign. Int/double call-site values reach the declaredP_FLOATslots through the same numeric compatibility the semantic analyzer already grants. The new table row matches the export.The test does not prove the source rectangle was respected. The PR already says that. An implementation that ignored
recand calledDrawTexturewould still exit 0. That is a coverage ceiling, not a lie about the wrapper.1. Ownership overview still lists two handle consumers
The architecture paragraph that explains the handle model still reads:
`rl_draw_texture` / `rl_unload_texture` take one back.That is now an incomplete census.
rl_draw_texture_recis a third function that takes a live index intog_textures[]. The table and the new section are correct; this sentence is the one that still describes yesterday's API. It is not a runtime bug. It is the kind of stale invariant sentence that the next wrapper will copy.VERDICT
The problem, such as it is, is documentation completeness in one sentence of the ownership model — not implementation, not ABI, not ownership at runtime. The wrapper does what it claims. The tests prove load-sentinel and a clean ASan exit, and they correctly refuse to claim more. CI will not run the windowed test (no raylib, no
$DISPLAYon the merge gate); that is the same leftover as #292, not a new hole in this patch.This environment could not run
make format-check(no clang-format),make test/make valgrindas a full suite, or the new windowed test (no raylib). The docs-table ↔ export check was run here: 24 names, no missing, no stale.Merge.
Sent by Cursor Automation: Code Reviewer
Review nit on #293, and a fair one. The ownership paragraph still read "rl_draw_texture / rl_unload_texture take one back", which was an incomplete census the moment rl_draw_texture_rec became a third consumer of g_textures[]. The table and the new section were already right; this sentence was describing yesterday's API. Says explicitly that the list is the complete census and that a new handle-taking wrapper belongs in it, so the next one does not inherit the same stale sentence.
|
Fixed the census nit — it was correct, and the "next wrapper will copy this sentence" framing is exactly why it was worth doing rather than merging past. The paragraph now lists all three handle consumers and says outright that it is the complete census, so the next one doesn't inherit the same staleness. Two notes on the rest of the review, since I agree with it and don't want the agreement to be silent:
Thanks for tracing declaration → /cursor-review |
There was a problem hiding this comment.
APPROVE
The census sentence now names rl_draw_texture, rl_draw_texture_rec, and rl_unload_texture, and says that list is the complete census of handle-taking wrappers. I counted against the source. Those three are the only functions that take a Brainrot handle into g_textures[], and all three use the same handle >= 0 && handle < BRAINRAY_MAX_TEXTURES && g_texture_used[handle] guard. rl_close_window bulk-unloads the table without taking a handle; it correctly stays out of that sentence.
Implementation is unchanged from the previous look. Declaration → STDROT_EXPORT_SIG (11/11, not variadic) → coerce_numeric into P_FLOAT / P_INT → .val.f / .val.i → Rectangle / Vector2 / make_color(args, 7) → DrawTextureRec still agree. Docs table ↔ 24 exports: no missing, no stale. Unary minus is a real production (UMINUS in lang.y), so the documented -64.0 is valid Brainrot, not a comment-only spelling.
The coverage ceiling on rec is unchanged: an implementation that ignored the source rectangle and called DrawTexture would still exit 0. The PR and the test docstring already say that. That is not a merge blocker and not a lie about the wrapper.
VERDICT
The previous leftover was documentation completeness in the ownership paragraph. It is closed. The wrapper still does what it claims. The tests still prove load-sentinel and a clean ASan exit, and they still correctly refuse to claim pixels or guard effectiveness.
This environment could not run make format-check (no clang-format), make test / make valgrind as a full suite, or the new windowed test (no raylib). make does not compile brainray/raylib.c; that is make brainray, which needs raylib. Docs-table ↔ export check was re-run here: 24 names, no missing, no stale. CI on this HEAD: lint pass, static-analysis pass.
Merge.
Sent by Cursor Automation: Code Reviewer


Description
rl_draw_textureblits a whole image, so an animation meant one file per frame and a sprite could not face the other direction.DrawTextureRectakes a source rectangle, which is what makes atlases, animation strips and tiled parallax possible from Brainrot at all. This is B2 in tung-tung-sahur's design doc, and the last thing blocking the rest of its M1.Two aggregates come apart here rather than one:
recis aRectangle(four floats),positionis aVector2(two floats). The float position differs fromrl_draw_texture's intx, y— that mirrors raylib rather than contradicting it (DrawTexturetakes ints,DrawTextureRectakes aVector2), and sub-pixel placement is what a scrolling background wants anyway.Negative
swmirrors horizontally, negativeshvertically — raylib's own idiom, since the sign of the source rectangle decides texture-coordinate order. UntilDrawTexturePro's rotation is exposed that's the only way to flip a sprite, so it's documented rather than left looking like a bug.Verified by eye
A four-frame atlas, sliced and flipped. Top row: frames 0–3 out of one 256×64 texture. Bottom: the same frame normal then with
sw = -64(arrow flips, top bar stays),sh = -64(bar moves to the bottom, arrow doesn't), a 32px half-slice, a tint, and two bad handles that correctly draw nothing.Both mirror claims in the docs are confirmed by that image, not assumed.
What the test does and does not establish
The last review was right to press on this, so I'll state it up front rather than have it found.
Verified — the assertion fails if the behaviour changes:
-1, the documented sentinel. This had no test before; flipping it to0makes the new test fail (confirmed);LeakSanitizerreport.Not verified, and not claimed:
recand blitting the whole texture would pass. Hence the visual check above.SO_CFLAGSis-fPIC -sharedwith no sanitizers, as for every shared object here includinglibstdrot.so, so an out-of-bounds read ofg_textures[]inside the module is invisible to ASan. I deleted the bounds check and the test still passed. The guards matchrl_draw_textureand keep a bogus GPU id away from raylib; they are reviewed, not tested.Closing either gap needs something outside this PR: a pixel-readback wrapper (
LoadImageFromTexture/TakeScreenshot) for the first, sanitizer-instrumented shared objects for the second. I didn't do the latter here because unsanitized.sos look like a deliberate repo-wide convention, not an oversight — happy to raise it separately if it isn't.The test writes its own 8×4 PNG from
zlib+struct, so the fixture adds no imaging dependency and nothing binary is checked in.Related Issue
Follows #291 (B1). No issue filed for B2 — say the word and I'll open one.
Type of Change
Checklist
make format-checklocally (ormake formatto fix)make test417 passed.make valgrind400 cases, 0 errors, exit 0.make format-checkclean.make cppchecknot run — not installed here; CI'sstatic-analysisjob is the first to see it.🤖 Generated with Claude Code