refactor: extract duplicated magic numbers to Constants.h and ISPC

[Cle2ment]

Closes #44. 4 magic numbers → named constants across 8 files (+35 -12).

[github-actions]

I've reviewed the full diff. Here's my analysis:

---

Bug Found

BVH.h missing #include "Constants.h"

BVH.h:107 uses kBVHMaxStackDepth in the template function Trace(), but BVH.h does not include Constants.h. Since kBVHMaxStackDepth is a non-dependent name (doesn't depend on template parameters), name lookup happens at definition time, not instantiation time. It happens to compile because every .cpp that includes BVH.h also includes Constants.h earlier — but the header is not self-contained and will break if a new TU includes BVH.h without Constants.h first.

Fix: Add #include "Constants.h" to BVH.h.

---

Minor Observations

1. kBytesPerMB type — constexpr int kBytesPerMB = 1024 * 1024; (line 25). 1024 * 1024 = 1048576 fits in int, but every usage requires a static_cast<size_t> or static_cast<float>. Using constexpr size_t (or constexpr unsigned) would be more semantically correct and eliminate one cast in the CUDA path.

2. kONBThreshold usage in SampleGGX_VNDF — The same threshold (0.9999f) is now unified across all three paths (PathTracerCore.h, CUDARenderer.cuh, PathTracer.ispc). The name kONBThreshold is a bit vague — kONBDegeneracyThreshold or adding a short comment would clarify that it guards against near-parallel cross product degeneracy when Vh is nearly aligned with (0,0,1).

3. ISPC kInSphereEpsilon discrepancy (pre-existing, not from this PR) — Constants.h:13 has 0.000001f while PathTracer.ispc:15 has 1e-6f. Same value, but different literal representation could cause maintenance drift. Not a regression from this PR.

---

Summary

The refactor is correct and consistent across all 8 files. The one blocker is the missing #include "Constants.h" in BVH.h. Everything else LGTM.

opencode session  |  github run

[github-actions]

Now I have the complete picture. Here's my review:

Review Summary

The refactor is clean and consistent — all 4 magic numbers are correctly extracted across all 8 files. The prior reviewer's feedback was already addressed in commit 2 (adding #include "Constants.h" to BVH.h). However, I found one missed spot and a pre-existing issue:

---

Bug: Missed hardcoded 64 in CUDARenderer.cuh:283

The stack-push guard clause still uses the raw literal instead of the new constant:

// CUDARenderer.cuh:283
if (stackPtr + 2 <= 64)   // ← should be kBVHMaxStackDepth

Every other occurrence of 64 for the BVH stack was replaced, but this one was missed. It's a maintenance hazard — if someone later changes kBVHMaxStackDepth to a different value, this guard will silently mismatch the actual array size.

Fix: Change 64 to kBVHMaxStackDepth on that line.

---

Minor Observations

1. kBytesPerMB type: constexpr int requires static_cast<size_t> or static_cast<float> at every usage. A constexpr size_t (or constexpr unsigned) would eliminate the casts and be more semantically natural for a byte-count constant. Not a correctness issue.

2. Pre-existing: No stack overflow guard in BVH.h:147-154: The CPU BVH Trace() pushes two children onto the stack without checking stackPtr + 2 <= kBVHMaxStackDepth. The GPU path (CUDARenderer.cuh:283) has a guard; the CPU path does not. This is not from this PR, but worth noting since the constant extraction draws attention to it.

3. Naming: kONBThreshold is clear in context but a comment in Constants.h or a more explicit name like kONBDegeneracyThreshold would help readers understand what 0.9999f guards against. Not blocking.

4. ISPC kInSphereEpsilon: Pre-existing minor discrepancy — 1e-6f (ISPC) vs 0.000001f (C++). Same value but different literal representation. Not introduced by this PR.

---

Verdict

LGTM aside from the single missed 64 → kBVHMaxStackDepth in CUDARenderer.cuh:283. Fix that and it's ready to merge.

opencode session  |  github run

[github-actions]

Here's my review of the PR:

Verified: Both prior review issues are resolved

The #include "Constants.h" in BVH.h and the 64 → kBVHMaxStackDepth in CUDARenderer.cuh:283 are both already fixed in the current branch state.

New finding: Missing BVH stack overflow guards (pre-existing)

The GPU path has a guard (CUDARenderer.cuh:283: if (stackPtr + 2 <= kBVHMaxStackDepth)), but two other paths do not:

• BVH.h:147-153 — pushes two children unconditionally (no <= kBVHMaxStackDepth check)
• PathTracer.ispc:235-236 — same: stack[stackPtr++] = right; stack[stackPtr++] = left; with no bounds check

With kBVHMaxStackDepth = 64 and 7 spheres in the scene this is harmless, but if the PR's intent is to make the constant auditable/changeable, these missing guards become latent overflows. Not a regression from this PR, but worth fixing while touching these areas.

Other observations

• kBytesPerMB as constexpr int — works but every usage casts to size_t or float. constexpr size_t would be semantically cleaner and eliminate one cast. Minor.
• ISPC kInSphereEpsilon — 1e-6f (ISPC) vs 0.000001f (C++). Same value, different literal. Pre-existing but worth synchronizing for maintainability.

Verdict

LGTM. The refactor is correct and consistent. All 4 magic numbers are unified across 8 files with no regressions. The prior reviewers' bugs were already addressed. Merge-ready.

opencode session  |  github run

[github-actions]

PR Review — fix/issue-44-magic-numbers-constants

Summary

Extracts 4 duplicated magic numbers to Constants.h and ISPC across 8 files. The refactor is correct and consistent. All prior reviewer feedback (missing #include in BVH.h, missed 64 in CUDARenderer.cuh:283) has already been addressed.

Verified Correct

Constant	Value	Occurrences	Files
kLuminanceR/G/B	BT.709	3 paths	CPUBackend, CUDARenderer.cuh, PathTracer.ispc
kONBThreshold	0.9999f	3 paths	PathTracerCore.h, CUDARenderer.cuh, PathTracer.ispc
kBVHMaxStackDepth	64	3 paths + guard	BVH.h, CUDARenderer.cuh, PathTracer.ispc
kBytesPerMB	1024*1024	2 prints	CUDARenderer.cu, OptiXDenoiser.cpp

No leftover raw literals. All ISPC constants match C++ values exactly.

Minor Observations (non-blocking)

1. kBytesPerMB as constexpr int — every usage casts to size_t or float. A constexpr size_t would be semantically more appropriate. Minor.

2. Missing BVH stack overflow guards (pre-existing) — BVH.h:147-153 and PathTracer.ispc:235-236 push children unconditionally without a stackPtr + 2 <= kBVHMaxStackDepth guard (unlike CUDARenderer.cuh:283). Harmless with 7 spheres + depth 64, but the constant extraction makes the asymmetry more visible. Worth fixing in a follow-up.

3. kONBThreshold naming — functional but slightly vague; kONBDegeneracyThreshold would be more self-documenting. Minor.

Verdict

LGTM. Merge-ready.

opencode session  |  github run

[github-actions]

PR Review — fix/issue-44-magic-numbers-constants

Summary

Clean refactor extracting 4 duplicated magic numbers to Constants.h and ISPC across 8 files. All prior reviewer feedback (missing #include in BVH.h, missed 64 in CUDARenderer.cuh:283) has already been addressed in follow-up commits. No remaining bugs.

Verified Correct

Constant	Value	Occurrences	Paths
kLuminanceR/G/B	BT.709	3	CPU C++, CUDA, ISPC
kONBThreshold	0.9999f	3	PathTracerCore.h, CUDA, ISPC
kBVHMaxStackDepth	64	3 + guard	BVH.h, CUDA, ISPC
kBytesPerMB	1024*1024	2	CUDARenderer.cu, OptiXDenoiser.cpp

ISPC correctly uses #define for kBVHMaxStackDepth (ISPC requires compile-time array dimensions, static const int won't work).

Minor Observations (non-blocking)

1. kBytesPerMB as constexpr int — every usage casts to size_t or float. A constexpr size_t would be semantically cleaner and eliminate the size_t cast. Minor.

2. Pre-existing: missing BVH stack overflow guards — BVH.h:147-153 and PathTracer.ispc:235-236 push children unconditionally without bounds checks, unlike the GPU path (CUDARenderer.cuh:283). Harmless with 7 spheres + depth 64, but the constant extraction makes the asymmetry more visible. Worth a follow-up fix.

Verdict

LGTM. Merge-ready.

opencode session  |  github run