feat(mcp): shader recompile observability#62
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 15 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds shader compilation status monitoring for RemoteControl inspection, updates contact-shadow intensity gating logic for ISL builds in the lighting shader, and refactors shader and C++ code across multiple files for readability and formatting consistency. New ChangesCode Quality and Feature Enhancements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds build-agnostic and profiling-only observability for shader (re)compiles, primarily to support MCP-driven hot-reload A/B testing and to make compile failures visible without log-diving. This is done by exposing shader-cache counters via inspect(kind="shadercache"), emitting Tracy timeline messages on compile success/failure, and introducing a small decoupling header so RemoteControl.cpp doesn’t need to include ShaderCache.h.
Changes:
- Add
ShaderCompileStatus.h+SIE::GetShaderCompileStatus()and expose it via MCPinspect(kind="shadercache"). - Emit Tracy timeline messages for shader compile success/failure (
#ifdef TRACY_ENABLE) fromShaderCache.cpp. - Apply formatting/cleanup-only changes across Weather Editor widgets, hooks, and several shader sources.
Reviewed changes
Copilot reviewed 9 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Features/RemoteControl.cpp | Adds inspect(kind="shadercache") and documents the new kind. |
| src/ShaderCompileStatus.h | New minimal header exposing shader compile counters without including ShaderCache.h. |
| src/ShaderCache.cpp | Implements GetShaderCompileStatus() and emits Tracy messages on compile success/failure. |
| src/ShaderCache.h | Minor formatting change; still contains global using namespace std::chrono;. |
| src/Hooks.cpp | Formatting-only change to __except braces. |
| src/Features/LightLimitFix.cpp | Include ordering + formatting-only changes. |
| src/Features/HDRDisplay.cpp | Indentation-only changes in a couple conditional blocks. |
| src/Utils/RestartSettings.h | Macro formatting tweak. |
| src/WeatherEditor/Weather/WeatherWidget.cpp | Formatting-only changes (if-bracing/tooltip style blocks/spacing). |
| src/WeatherEditor/Weather/PrecipitationWidget.cpp | Indentation/formatting-only changes. |
| src/WeatherEditor/Weather/LightingTemplateWidget.cpp | Formatting-only changes in searchable settings list. |
| src/WeatherEditor/Weather/CellLightingWidget.cpp | Formatting-only changes in inherited-style handling and search list. |
| package/Shaders/Lighting.hlsl | Preprocessor directive indentation change only. |
| package/Shaders/ISTemporalAA.hlsl | Formatting-only changes (semantic spacing, preprocessor indentation, alignment). |
| features/Water Effects/Shaders/WaterEffects/WaterCaustics.hlsli | Formatting-only change to ternary expressions. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package/Shaders/Lighting.hlsl (1)
1-1: PR title could better reflect the shader gating change.Current title is valid Conventional Commit format, but it doesn’t mention the ISL contact-shadow gating adjustment included here. Consider:
feat(shader): add isl contact-shadow intensity gating(or include both MCP + shader scope in the body).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/Shaders/Lighting.hlsl` at line 1, The PR title doesn't mention the ISL contact-shadow intensity gating introduced in the shader; update the commit/PR title to explicitly reference the shader gating change (for example: "feat(shader): add isl contact-shadow intensity gating") and, if needed, expand the PR body to note both the MCP and shader scope so reviewers can quickly see the change affects Lighting.hlsl (macro/guard LIGHTING) and the ISL contact-shadow logic.package/Shaders/ISTemporalAA.hlsl (1)
41-41: 💤 Low valueConsider standard preprocessor directive spacing.
The space after
#in# defineis unconventional in HLSL/C preprocessor directives. Standard style uses#definewithout space.🎨 Proposed fix to use standard preprocessor formatting
-# define cmp - +# define cmp -Note: Both forms are valid, but
#define(no space) is the established convention.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package/Shaders/ISTemporalAA.hlsl` at line 41, The preprocessor directive for the macro "cmp" uses nonstandard spacing ("# define cmp -"); change it to the conventional form by removing the space after the hash so the directive reads "`#define` cmp -" in ISTemporalAA.hlsl (search for the "cmp" macro definition to locate it).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@package/Shaders/ISTemporalAA.hlsl`:
- Line 41: The preprocessor directive for the macro "cmp" uses nonstandard
spacing ("# define cmp -"); change it to the conventional form by removing the
space after the hash so the directive reads "`#define` cmp -" in ISTemporalAA.hlsl
(search for the "cmp" macro definition to locate it).
In `@package/Shaders/Lighting.hlsl`:
- Line 1: The PR title doesn't mention the ISL contact-shadow intensity gating
introduced in the shader; update the commit/PR title to explicitly reference the
shader gating change (for example: "feat(shader): add isl contact-shadow
intensity gating") and, if needed, expand the PR body to note both the MCP and
shader scope so reviewers can quickly see the change affects Lighting.hlsl
(macro/guard LIGHTING) and the ISL contact-shadow logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d2bdb673-e144-4b53-8653-f3e0e4e34a43
📒 Files selected for processing (15)
features/Water Effects/Shaders/WaterEffects/WaterCaustics.hlslipackage/Shaders/ISTemporalAA.hlslpackage/Shaders/Lighting.hlslsrc/Features/HDRDisplay.cppsrc/Features/LightLimitFix.cppsrc/Features/RemoteControl.cppsrc/Hooks.cppsrc/ShaderCache.cppsrc/ShaderCache.hsrc/ShaderCompileStatus.hsrc/Utils/RestartSettings.hsrc/WeatherEditor/Weather/CellLightingWidget.cppsrc/WeatherEditor/Weather/LightingTemplateWidget.cppsrc/WeatherEditor/Weather/PrecipitationWidget.cppsrc/WeatherEditor/Weather/WeatherWidget.cpp
|
No actionable suggestions for changed features. |
Read completedTasks/totalTasks once and derive `compiling` from that single snapshot so callers never observe compiling=false with work still outstanding; switch to named-field init so member reordering can't silently misassign. Also list the `plugin` field in the inspect(kind='shadercache') description to match the actual response. Addresses Copilot review on PR #62. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Surface shader (re)compile state two ways from one event so hot-reload
A/B testing stops relying on guesswork.
MCP (build-agnostic, drives the workflow):
- New inspect(kind='shadercache') on the RemoteControl server returns
{ compiling, completedTasks, totalTasks, failedTasks,
currentFailedCount, frame_count }, built from existing thread-safe
ShaderCache accessors (no new state on the hot class). Poll
completedTasks against a pre-deploy snapshot to know a hot-reloaded
shader finished recompiling; a rising failedTasks / currentFailedCount
surfaces a failed compile that was previously invisible.
Tracy (profiling-only, precise correlation):
- TracyMessage on success / red TracyMessageC on failure at both
CompileShader completion points, #ifdef TRACY_ENABLE-guarded (no
formatting cost in normal builds). Gives the exact frame so A/B perf
windows split on the recompile via get_messages().
To keep RemoteControl.cpp free of ShaderCache.h (whose global-scope
`using namespace std::chrono` leaks chrono names into the vendored
cpp-mcp httplib/base64 headers and trips warning-as-error), the status
read is exposed through a tiny dependency-free header
(ShaderCompileStatus.h) implemented in ShaderCache.cpp.
Validated with an ALL-TRACY build.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Read completedTasks/totalTasks once and derive `compiling` from that single snapshot so callers never observe compiling=false with work still outstanding; switch to named-field init so member reordering can't silently misassign. Also list the `plugin` field in the inspect(kind='shadercache') description to match the actual response. Addresses Copilot review on PR #62. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alandtse
force-pushed
the
feat/shader-recompile-observability
branch
from
c5e05c6 to
4563d40
Compare
|
✅ A pre-release build is available for this PR: |
Summary
Surfaces shader (re)compile state two ways from one event, so hot-reload A/B testing stops relying on guesswork.
MCP (build-agnostic) — new
inspect(kind='shadercache')Returns
{ compiling, completedTasks, totalTasks, failedTasks, currentFailedCount, frame_count }, built entirely from existing thread-safeShaderCacheaccessors (no new state on the hot class). PollcompletedTasksagainst a pre-deploy snapshot to confirm a hot-reloaded shader recompiled; a risingfailedTasks/currentFailedCountsurfaces a failed compile that was previously invisible (no log-diving).Tracy (profiling builds) — recompile messages
TracyMessageon success / redTracyMessageCon failure at bothCompileShadercompletion points,#ifdef TRACY_ENABLE-guarded (no formatting cost in normal builds). Gives the exact frame so A/B perf windows split on the recompile viaget_messages().Decoupling (
ShaderCompileStatus.h)RemoteControl.cppmust not includeShaderCache.h— its global-scopeusing namespace std::chronoleaks chrono names (e.g.last) into the vendored cpp-mcp httplib/base64 headers and trips warning-as-error. The status read is exposed through a tiny dependency-free header implemented inShaderCache.cpp.Validation
ALL-TRACYbuild green.inspect(shadercache)counters climbed through a 3455-task cache rebuild (compiling: true, 0 failures), and Tracy showed"Shader compiled: …"messages firing per compile alongsideShaderCompilationTask::Performzones.Notes
Success messages fire for every compile, so a cold-cache startup emits many (early-timeline, Tracy-only) — acceptable; a follow-up could gate on steady state if it's noise. The MCP counters are the primary, build-agnostic signal.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Refactor