fix: removed duplicate perframe buffer setup in prepass

[soda3000]

Summary by CodeRabbit

• Refactor

  • Simplified render initialization by removing a redundant render-target rebinding step and related per-frame buffer rebinds in the prepass path.

• New Features / Improvements

  • Improved shader compilation timing: record compilation completion time, reset timing on cache clear, and provide more accurate elapsed/ETA and progress reporting with safer synchronization.

[coderabbitai]

Walkthrough

Removed render-target rebind and CS constant-buffer binding from Deferred::PrepassPasses(). Added completionTime to SIE::ShaderCache::CompilationSet, set/reset it on compilation completion/clear, and adjusted ShaderCache timing and logging to use completionTime (or current time) for elapsed/ETA reporting.

Changes

Cohort / File(s)	Summary
Deferred prepass cleanup src/Deferred.cpp	Removed code in Deferred::PrepassPasses() that set the render-target dirty flag and performed OMSetRenderTargets rebinding and CSSetConstantBuffers (including conditional VR buffer binding).
Shader cache timing src/ShaderCache.cpp, src/ShaderCache.h	Added LARGE_INTEGER completionTime to SIE::CompilationSet (initialized to zero). In ShaderCache.cpp, set completionTime when all tasks finish/failed (guarded double-check), reset it on cache clear, and compute elapsed/total ms using lastReset → completionTime (or current time) for reporting and ETA adjustments. Also moved completion logging outside the lock and deferred logging logic.

Sequence Diagram(s)

sequenceDiagram
  participant Prepass as Deferred::PrepassPasses()
  participant RT as RenderTargets/OMSetRenderTargets
  participant CS as CS::SetConstantBuffers
  rect rgba(200,230,255,0.3)
    Note over Prepass,RT: Previous flow (before change)
    Prepass ->> RT: mark DIRTY_RENDERTARGET + OMSetRenderTargets
    Prepass ->> CS: CSSetConstantBuffers(per-frame, VR if active)
  end
  rect rgba(240,240,240,0.3)
    Note over Prepass,RT: New flow (after change)
    Prepass --x RT: (no rebind)
    Prepass --x CS: (no CS constant buffer binding)
  end

Loading

sequenceDiagram
  participant ShaderCache as ShaderCache
  participant Timer as QueryPerformanceCounter
  rect rgba(230,255,230,0.3)
    Note over ShaderCache,Timer: Compilation timing lifecycle
    ShaderCache ->> Timer: lastReset set (start)
    ShaderCache ->> Timer: (periodic) update totalTime
    alt all tasks completed/failed and completionTime == 0
      ShaderCache ->> Timer: set completionTime (now)  %% guarded double-check
      ShaderCache ->> ShaderCache: schedule logging (now - lastReset)
    end
    ShaderCache ->> Timer: on Clear/Reset -> completionTime = 0
  end

Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

• Pay attention to render-target and constant-buffer binding relocation to ensure no regressions, especially VR paths.
• Inspect thread-safety around completionTime updates and logging (double-checked guard and lock usage).
• Confirm tests/logging cover concurrent completion and Clear() races.

Possibly related PRs

• perf(shadercache): use faster clock #1275 — touches CompilationSet timing fields and similar ShaderCache timing/reset behavior.

Suggested reviewers

• doodlum
• alandtse
• Pentalimbed

Poem

🐰 I hopped through code with nimble paws,

Snipped rebinding from prepass laws,
Timers now tick a tidy chime,
Completion logged in proper time,
A quiet hop, a cleaner cause.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run @coderabbitai generate docstrings to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check	✅ Passed	The pull request title "fix: removed duplicate perframe buffer setup in prepass" directly and accurately describes the primary change in Deferred.cpp, where a block setting render target state to dirty and binding CS constant buffers in PrepassPasses() was removed. The title is clear, specific, and concise. While the PR also includes supporting changes to ShaderCache.cpp and ShaderCache.h for completion time tracking, the PR instructions note that titles don't need to cover every detail of a changeset. The title accurately reflects the main change from the developer's perspective.

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7e8611c and a72cdf3.

📒 Files selected for processing (2)

• src/ShaderCache.cpp (4 hunks)
• src/ShaderCache.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (5)

src/ShaderCache.h (1)

247-247: LGTM! Clean addition for tracking compilation completion time.

The new completionTime member follows the same pattern as other timing fields and is properly initialized to zero.

Also applies to: 256-256

src/ShaderCache.cpp (4)

2728-2750: LGTM! Properly implemented double-checked locking.

The completion time tracking uses correct double-checked locking:

• Optimization check outside lock to avoid contention
• Re-check with lock held to prevent race conditions
• Logging performed outside lock

This addresses the race condition flagged in previous reviews.

---

2765-2765: LGTM! Proper reset of completion time.

The completionTime is correctly reset to zero alongside other timing state, with proper synchronization via compilationMutex.

---

2783-2784: Good clarification on ETA calculation.

The comments helpfully explain that ETA uses active compilation time (totalTime) rather than wall-clock elapsed time, which is the correct approach for rate-based estimation.

---

2797-2803: Clean implementation of elapsed time calculation.

The code correctly uses completionTime when available (compilation finished) or currentTime when still compiling. This provides accurate wall-clock elapsed time for reporting.

Note: The read of completionTime.QuadPart at line 2802 is unlocked, but this is acceptable since it's only used for stats/logging and the value transitions atomically from 0 to a timestamp on 64-bit systems.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

Using provided base ref: 8f51609
Using base ref: 8f51609
Base commit date: 2025-10-25T21:51:23+01:00 (Saturday, October 25, 2025 09:51 PM)
No actionable suggestions for changed features.

[github-actions]

✅ A pre-release build is available for this PR:
Download

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/ShaderCache.h (1)

247-247: Consider thread safety for completionTime access.

The completionTime field will be accessed from multiple threads (written in Complete(), read in GetStatsString()) without explicit synchronization. While LARGE_INTEGER operations are typically atomic on x64, consider documenting this assumption or adding explicit synchronization if strict ordering guarantees are needed.

src/ShaderCache.cpp (1)

2667-2696: Consider clarifying mixed time base semantics.

The GetStatsString() method now mixes two different time bases:

• totalMs is wall-clock elapsed time (line 2675)
• GetEta() returns estimates based on active compilation time rate (line 2662)

At line 2685, the estimated completion is calculated as GetEta() + totalMs, which combines these different time bases. While this might be the intended behavior (wall-clock elapsed + active-rate-based remaining time), it could be confusing:

• If compilation is throttled or paused, wall-clock time grows faster than active time
• The ETA would be based on active rate, not wall-clock rate
• Users might see "10 minutes elapsed / 12 minutes total" when it actually might take longer in wall-clock time

Consider either:

1. Adding a comment explaining this mixed-base calculation
2. Using wall-clock rate for ETA as well for consistency
3. Displaying both timing types separately (e.g., "10min elapsed, 2min work remaining")

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between b033023 and 7e8611c.

📒 Files selected for processing (2)

• src/ShaderCache.cpp (4 hunks)
• src/ShaderCache.h (2 hunks)

🧰 Additional context used

📓 Path-based instructions (2)

**/*.{cpp,cxx,cc,c,h,hpp,hxx,hlsl,hlsli,fx,fxh,py}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Do not include TODO/FIXME placeholders; provide complete, working solutions

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

src/**/*.{cpp,cxx,cc,h,hpp,hxx}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

src/**/*.{cpp,cxx,cc,h,hpp,hxx}: Ensure SE/AE/VR runtime compatibility; use runtime detection patterns (e.g., REL::RelocateMember())
Include robust error handling and resource management with graceful degradation in the plugin code

Files:

• src/ShaderCache.h
• src/ShaderCache.cpp

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Build plugin and addons

🔇 Additional comments (3)

src/ShaderCache.h (1)

251-257: LGTM!

The constructor properly initializes all timing fields including the new completionTime field. The zero-initialization clearly indicates "not yet completed" status.

src/ShaderCache.cpp (2)

2625-2639: LGTM!

The Clear() method properly resets all timing state including the new completionTime field. The reset is correctly synchronized under compilationMutex.

---

2653-2665: LGTM!

The added comments clarify the important distinction between active compilation time (used for ETA) and wall-clock elapsed time (used for display). This distinction is valuable given the dual-timing semantics introduced in this PR.

[alandtse]

Learn to revert commits or modify your commit history instead of creating new branches. You complicate the history if we have comments/reviews and then you start a new branch.