Fix negative coordinates in tiled image sampling - #598
Merged
Merged
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
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.
Rotating a
TiledImagePaintcan produce negative source coordinates. The wrapped sampler uses signedmod, leaving negative pixel indexes that raiseIndexDefectin debug builds and cause misplaced pixels or out-of-bounds reads in release builds. The example in #597 reproduces both symptoms on current master.Wrap the base X/Y coordinates with an expanded integer template, following janAkali's diagnosis in the forum discussion. Image dimensions are positive, so a negative remainder can be corrected by adding the dimension. Derive the two adjacent indexes with boundary checks, reducing the sampler to two modulo operations without out-of-line wrapping calls.
Add regression tests comparing wrapped sampling with an explicitly repeated image, covering negative and fractional coordinates, tile seams, non-square and single-pixel dimensions, and five rotation angles including the reported -7 degrees. Include the tests in the standard runner and add
tests/bench_images_tiled.nimfor repeatable rendering/sampler benchmarks.Fixes #597.
Validation on Windows with Nim 2.2.10:
git diff --checkpasses.Performance on a Ryzen 9 5900X, Nim 2.2.10, Windows amd64, GCC 11.1.0,
-d:release/-O3, no LTO: six interleaved rounds pinned to one logical CPU, with warmup and matching pixel checksums. Times are median milliseconds per operation. Shared comparisons use nonnegative source coordinates so the pre-fix build does not perform invalid reads.Disassembly confirms the initial
euclModimplementation leaves four external calls per wrapped sample; the revised sampler has none. The function variant also changes the compiler's placement of thread-local exception-flag lookups and happens to accelerate textured/non-tiled workloads. The revised version does not preserve every incidental gain of that variant; it stays close to the original baseline on the measured workloads. Results are specific to this compiler/platform.Run the benchmark with
nim c -r -d:release -d:benchAffinity tests/bench_images_tiled.nim. Appendnegativeto include the original failing-coordinate case; that case must only be run against fixed builds.