Skip to content

Add UV texturing: checker and image textures with bilinear filtering - #1

Open
erichanwang wants to merge 2 commits into
masterfrom
feature/textures
Open

Add UV texturing: checker and image textures with bilinear filtering#1
erichanwang wants to merge 2 commits into
masterfrom
feature/textures

Conversation

@erichanwang

Copy link
Copy Markdown
Owner

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

  • Sphere and Plane now compute UV on every hit (standard spherical
    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 (a Texture*) replaces isGrid/gridColor1/
    gridColor2/gridSize entirely.
  • CheckerTexture: the procedural checker, migrated from the old
    world-space formula onto UV.
  • ImageTexture: loads a PPM (P3 ascii or P6 binary) and samples it with
    bilinear filtering, wrapping UV instead of clamping.
  • main.cpp and main_control_demo.cpp migrated off isGrid onto
    CheckerTexture(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)

  1. Render cost of the texture pointer. Material (and the HitRecord
    holding 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 ground
    plane, on otherwise identical code. Switched to a raw Texture*, matching
    this codebase's existing style of never freeing Object* either (see
    Scene::addObject) - textures live for the process lifetime, same as the
    objects referencing them. Re-measured afterward: ~0.5s both ways,
    texture path sometimes faster, i.e. within noise.

  2. Plane UV sign convention. The first tangent-basis choice reproduced
    the old floor(point/gridSize) checker parity correctly for generic
    points, 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 equator
    points (u,v are exactly computable there).
  • testPlaneCheckerMatchesOldGridFormula: checks the new UV-driven
    checker 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 PPM
    fixture 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.cpp all build clean under
-std=c++17 -Wall -Wextra -Werror -O2 -pthread, as does the test binary.
Rendered src/main.cpp and inspected the output PPM - ground plane checker
is visually identical to before, and it's correctly visible reflected in
the chrome and glass spheres.

What to check

  • The two commit-message pitfalls above are exactly what a reviewer should
    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.cpp still references the old isGrid/gridColor1/2/
    gridSize fields and will fail to build until it's migrated onto
    Texture - left untouched here since another change is in flight
    against bench/.
  • Not attempted: normal/bump mapping (the suggested follow-on if time
    allowed) - ran out of scope/time budget for this pass.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant