Add UV texturing: checker and image textures with bilinear filtering - #1
Open
erichanwang wants to merge 2 commits into
Open
Add UV texturing: checker and image textures with bilinear filtering#1erichanwang wants to merge 2 commits into
erichanwang wants to merge 2 commits into
Conversation
Materials could previously only render flat color or the isGrid special case bolted directly onto Material (gridColor1/2, gridSize). Sphere and Plane now compute UV at each hit (Block gets it almost for free from its existing per-face normal), and Material holds a Texture instead: a CheckerTexture (procedural, replaces isGrid) or an ImageTexture that reads a PPM file (P3 or P6) with bilinear filtering. main.cpp and main_control_demo.cpp are migrated off isGrid onto CheckerTexture and render pixel-identical to before. Pitfall worth flagging: Material.texture started as a std::shared_ptr, which looked like the obvious ownership-safe choice. It measurably cost 2.2x render time (0.8s vs 0.36s on main.cpp's scene) because HitRecord (and its embedded Material) gets copied by value on every single ray-object test in Scene/BVH traversal, and a shared_ptr copy does an atomic refcount bump even when nothing else changes hands. Switched to a raw Texture* to match this codebase's existing style of never freeing Objects (see Scene::addObject) - textures live for the process lifetime same as the objects that reference them. That took the cost back down to within measurement noise (~0.5s both ways, texture path sometimes faster). Plane UV needed a second pass too: the first tangent-basis choice matched the old floor(point/gridSize) checker pattern for generic points but silently flipped phase for any point sitting exactly on a grid line boundary (an integer world coordinate), because the sign-flip trick it relied on only preserves floor() parity for non-integer inputs. Fixed by choosing axes so the ground plane's UV equals its world x,z exactly, no sign flip involved - the closed-form parity match now holds everywhere, not just generically. Added testSphereUVMapping (closed-form UV at poles/equator), testPlane- CheckerMatchesOldGridFormula (regression covering the boundary case above), and testBilinearImageTexture (hand-computed interpolation over a 2x2 PPM fixture). All 18 tests pass; the four targets and the test binary build clean under -Wall -Wextra -Werror. Not done: normal/bump mapping (the suggested follow-on) and updating bench/benchmark.cpp's isGrid usage - both out of scope for this change.
Material lost isGrid/gridColor1/gridColor2/gridSize when textures replaced them, which left bench/benchmark.cpp referencing members that no longer exist, so the Benchmark target did not compile on this branch at all. The checker texture is a function-local static rather than a heap allocation because a Material only holds a raw Texture*, and the benchmark builds its scene once and never tears it down - the same way it already treats the Object* it allocates.
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.
Why
Material only rendered flat color or the isGrid special case (gridColor1/2,
gridSize) bolted directly onto Material - a one-off that couldn't be
extended to anything else. This replaces it with a proper Texture
abstraction Material can hold.
What changed
parameterization for Sphere; an orthonormal tangent basis for Plane).
Block gets box-mapped UV nearly for free from its existing per-face
normal logic.
Material::texture(aTexture*) replacesisGrid/gridColor1/gridColor2/gridSizeentirely.CheckerTexture: the procedural checker, migrated from the oldworld-space formula onto UV.
ImageTexture: loads a PPM (P3 ascii or P6 binary) and samples it withbilinear filtering, wrapping UV instead of clamping.
main.cppandmain_control_demo.cppmigrated offisGridontoCheckerTexture(gridColor1, gridColor2, 1.0)- same checker pattern,verified pixel-identical by eye against the prior render.
Two things that looked right and weren't (both fixed, both covered by new tests)
Render cost of the texture pointer.
Material(and theHitRecordholding it) gets copied by value on every ray-object test throughout
Scene/BVH traversal. A first pass held the texture in a
std::shared_ptr,which looked like the obviously safe choice - but its atomic refcount
bump on every one of those copies cost real time: measured 0.8s vs 0.36s
(~2.2x) rendering
main.cpp's scene with vs without a textured groundplane, on otherwise identical code. Switched to a raw
Texture*, matchingthis codebase's existing style of never freeing
Object*either (seeScene::addObject) - textures live for the process lifetime, same as theobjects referencing them. Re-measured afterward: ~0.5s both ways,
texture path sometimes faster, i.e. within noise.
Plane UV sign convention. The first tangent-basis choice reproduced
the old
floor(point/gridSize)checker parity correctly for genericpoints, but silently flipped phase for any point sitting exactly on a
grid-line boundary (an integer world coordinate) - the sign-flip trick
it relied on only preserves floor() parity for non-integer inputs, not
at the boundary. Fixed by picking axes so the ground plane's UV equals
its world x,z exactly (no sign flip at all), which matches the old
formula bit-for-bit including at boundaries.
Tests added (tests/test_math_physics.cpp)
testSphereUVMapping: closed-form UV at both poles and four equatorpoints (u,v are exactly computable there).
testPlaneCheckerMatchesOldGridFormula: checks the new UV-drivenchecker against the old world-space formula across a grid of points,
deliberately including exact grid-line boundaries (this is the
regression test for pitfall Replace O(n^2) broad phase with a spatial hash; fix box collision #2 above).
testBilinearImageTexture: hand-computed interpolation over a 2x2 PPMfixture written to
build/and read back - covers the center point(equal-weighted average of all 4 texels) and an edge-midpoint (pure
horizontal blend, zero vertical blend).
All 18 tests pass.
src/main.cpp,main_blocks.cpp,main_parachutes.cpp,and
main_control_demo.cppall build clean under-std=c++17 -Wall -Wextra -Werror -O2 -pthread, as does the test binary.Rendered
src/main.cppand inspected the output PPM - ground plane checkeris visually identical to before, and it's correctly visible reflected in
the chrome and glass spheres.
What to check
scrutinize: the raw-pointer ownership tradeoff (fine here, given the
codebase's existing style, but worth a second opinion), and the boundary
case in
testPlaneCheckerMatchesOldGridFormula.bench/benchmark.cppstill references the oldisGrid/gridColor1/2/gridSizefields and will fail to build until it's migrated ontoTexture- left untouched here since another change is in flightagainst
bench/.allowed) - ran out of scope/time budget for this pass.