feat(truepbr): add vertex ambient occlusion slider#2329
Conversation
|
No actionable suggestions for changed features. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a float VertexAOStrength setting to TruePBR (UI slider + JSON persistence), packs it into the feature buffer, exposes it in SharedData::FeatureData, and uses it in Lighting.hlsl to remap vertexAO and adjust vertex-color/emissive handling in TRUE_PBR paths. ChangesTruePBR Vertex AO Strength
Sequence Diagram(s)sequenceDiagram
participant ImGui as ImGui DrawSettings
participant Settings as globals::features::truePBR.settings
participant FB as GetFeatureBufferData
participant CB as Shader FeatureData cbuffer
participant PS as Lighting pixel shader
ImGui->>Settings: adjust VertexAOStrength (slider)
Settings->>FB: read settings for packing
FB->>CB: pack/send FeatureData.truePBRSettings to GPU
CB->>PS: pixel shader reads VertexAOStrength
PS->>PS: remap vertexAO = lerp(1, vertexAO, VertexAOStrength)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/TruePBR.h (1)
50-54: ⚡ Quick winConsider zero-initializing the padding array.
The
pad[3]array lacks a default member initializer. WhileRestoreDefaultSettings()does zero-initialize the entire struct via aggregate initialization ({}), a newSettingsinstance created without explicit initialization will have uninitialized padding bytes. This won't affect shader functionality (padding is not read), but could lead to non-deterministic behavior when copying or comparing structs.♻️ Suggested initialization
struct alignas(16) Settings { uint DisableVertexAO = false; - uint pad[3]; + uint pad[3] = {}; };🤖 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 `@src/TruePBR.h` around lines 50 - 54, The Settings struct's padding array pad[3] is not default-initialized which can leave those bytes indeterminate; update the struct definition (struct Settings) to provide a default initializer for pad (e.g. pad[3] = {} ) so any new Settings() has deterministically zeroed padding; this keeps RestoreDefaultSettings() behavior but prevents non-deterministic copies or comparisons.
🤖 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 `@src/TruePBR.h`:
- Around line 50-54: The Settings struct's padding array pad[3] is not
default-initialized which can leave those bytes indeterminate; update the struct
definition (struct Settings) to provide a default initializer for pad (e.g.
pad[3] = {} ) so any new Settings() has deterministically zeroed padding; this
keeps RestoreDefaultSettings() behavior but prevents non-deterministic copies or
comparisons.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b141c1f0-5413-4c7c-b645-2d3ad2cc0989
📒 Files selected for processing (5)
package/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/FeatureBuffer.cppsrc/TruePBR.cppsrc/TruePBR.h
|
wouldn't a slider be better for this? no vetex AO can also look bad |
|
✅ A pre-release build is available for this PR: |
jiayev
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/TruePBR.cpp (1)
43-45: PR metadata suggestion: align title/body with the current implementation.Since the feature now exposes strength control (not only disable), a clearer title would be:
feat(truepbr): add vertex ao strength setting. If tracked by an issue, consider addingImplements #<id>orAddresses #<id>in the PR description.As per coding guidelines
**/*: "When reviewing PRs, please provide suggestions for Conventional Commit Titles ... and Issue References".🤖 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 `@src/TruePBR.cpp` around lines 43 - 45, The PR title/body should be updated to reflect the new feature exposing vertex AO strength: change the commit/PR title to something like "feat(truepbr): add vertex ao strength setting" and update the description to mention TruePBR::Settings and the VertexAOStrength field; if this change implements or addresses an issue, add "Implements #<id>" or "Addresses #<id>" in the PR body to satisfy the Conventional Commit / PR metadata guideline.
🤖 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.
Inline comments:
In `@src/TruePBR.cpp`:
- Around line 317-320: LoadSettings currently copies user JSON blindly into
settings which can propagate malformed or out-of-range VertexAOStrength into
shaders; change LoadSettings to explicitly read VertexAOStrength from o_json
(e.g. check for key, ensure it's a number), convert to a float, clamp it to a
safe range (e.g. 0.0f–1.0f) and assign that value into
settings["VertexAOStrength"] (or the struct member you use), falling back to a
known default if missing or invalid; reference the LoadSettings function and the
VertexAOStrength setting when making this change.
---
Nitpick comments:
In `@src/TruePBR.cpp`:
- Around line 43-45: The PR title/body should be updated to reflect the new
feature exposing vertex AO strength: change the commit/PR title to something
like "feat(truepbr): add vertex ao strength setting" and update the description
to mention TruePBR::Settings and the VertexAOStrength field; if this change
implements or addresses an issue, add "Implements #<id>" or "Addresses #<id>" in
the PR body to satisfy the Conventional Commit / PR metadata guideline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 056d0bdc-efb5-4936-877c-ac1dfd620b85
📒 Files selected for processing (4)
package/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslsrc/TruePBR.cppsrc/TruePBR.h
|
This does almost nothing? But at least this way it doesn't change the color of trees etc. |
fixed |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@package/Shaders/Lighting.hlsl`:
- Around line 2158-2159: Clamp SharedData::truePBRSettings.VertexAOStrength to
[0,1] before using it in any lerp/extrapolation so the shader cannot over- or
under-apply AO; specifically, compute a local float (e.g.,
clampedVertexAOStrength) = clamp(SharedData::truePBRSettings.VertexAOStrength,
0.0, 1.0) and use that variable in the lerp that modifies pbrVertexColor (the
expression around pbrVertexAO / pbrVertexColor), and apply the same fix to the
other similar uses referenced in this review (the uses around the other
VertexAOStrength sites).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e3c0ca50-cf95-48b7-b678-58e4b4228357
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
alandtse
changed the title
|
@coderabbitai what's the standard scope we use for pbr? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2933
So the current PR title |
alandtse
changed the title
…#2329) Co-authored-by: Skrubby Skrub In A Shrub <87662196+SkrubbySkrubInAShrub@users.noreply.github.com>
4 participants
This pull request introduces a configurable Vertex Ambient Occlusion (AO) strength parameter for the TruePBR rendering system, making AO blending more flexible and user-tunable. The main changes include adding a new
VertexAOStrengthsetting, updating the shader and UI to use this parameter, and implementing serialization for the new setting.TruePBR Settings Enhancements:
VertexAOStrengthparameter toTruePBR::Settings, exposed in the UI via an ImGui slider, allowing users to control the influence of vertex AO in the shader. [1] [2]SaveSettings,LoadSettings,RestoreDefaultSettings) forTruePBR::Settingsto support saving/loading user preferences. [1] [2]Shader Integration:
TruePBRSettingsstruct inSharedData.hlsliand added it to theFeatureDataconstant buffer, makingVertexAOStrengthavailable to shaders. [1] [2]Lighting.hlslto blend vertex AO using the newVertexAOStrengthparameter, providing finer control over AO application in TruePBR mode.Summary by CodeRabbit
New Features
Visual / Lighting