feat: vanilla fresnel#2332
Draft
jiayev wants to merge 38 commits into
Draft
Conversation
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 VanillaFresnel feature with runtime settings and UI, exposes settings to shaders, implements GGX microfacet specular helpers and a VANILLA_FRESNEL lighting path, updates material/env handling in Lighting.hlsl, and switches grass rendering to use roughness and F0 for specular/reflectance. ChangesVanilla Fresnel Feature
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/FeatureBuffer.cpp (1)
39-56:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix feature-buffer packing order to match
SharedData::FeatureData.Line 53 inserts
vanillaFresnel.settingsbefore linear/terrain/fog blocks, but HLSL expectsvanillaFresnelSettingsafterexponentialHeightFogSettings. This corrupts constant-buffer decoding for multiple settings blocks.Proposed fix
return _GetFeatureBufferData( globals::features::grassLighting.settings, globals::features::extendedMaterials.settings, globals::features::dynamicCubemaps.settings, globals::features::terrainShadows.GetCommonBufferData(), globals::features::lightLimitFix.GetCommonBufferData(), globals::features::wetnessEffects.GetCommonBufferData(), globals::features::skylighting.GetCommonBufferData(a_inWorld), globals::features::cloudShadows.settings, globals::features::lodBlending.settings, globals::features::hairSpecular.settings, globals::features::terrainVariation.settings, globals::features::ibl.GetCommonBufferData(), globals::features::extendedTranslucency.GetCommonBufferData(), - globals::features::vanillaFresnel.settings, globals::features::linearLighting.GetCommonBufferData(), globals::features::terrainBlending.settings, - globals::features::exponentialHeightFog.settings); + globals::features::exponentialHeightFog.settings, + globals::features::vanillaFresnel.settings); }🤖 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/FeatureBuffer.cpp` around lines 39 - 56, The call to _GetFeatureBufferData in FeatureBuffer.cpp passes vanillaFresnel.settings in the wrong position, breaking alignment with HLSL's SharedData::FeatureData; update the argument order so that globals::features::vanillaFresnel.settings is moved from before linearLighting/terrainBlending/exponentialHeightFog to be the last argument (i.e., placed after globals::features::exponentialHeightFog.settings) so the C++ packing matches the HLSL structure.
🤖 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 `@features/Grass` Lighting/Shaders/GrassLighting/GrassLighting.hlsli:
- Around line 11-17: The call to BRDF::Vis_SmithJointApprox has its NdotV/NdotL
parameters reversed and the dot products lack epsilon clamping; update the code
so NdotL and NdotV are computed with epsilon-protected clamping (use the same
pattern as LightingEval.hlsli / PBRMath.hlsli, e.g. clamp with a small EPS like
max(saturate(dot(...)), EPS) for NdotL and NdotV) and call
BRDF::Vis_SmithJointApprox(roughness, NdotV, NdotL) with the corrected parameter
order; keep the existing BRDF::D_GGX call and other dot products (NdotH, VdotH)
consistent.
In `@package/Shaders/Common/LightingEval.hlsli`:
- Around line 149-150: The energy compensation uses specularBRDF =
BRDF::EnvBRDF(...) and then divides by (specularBRDF.x + specularBRDF.y), which
can be zero and produce inf/NaN; fix by guarding the denominator with a small
epsilon (e.g. use max(sum, 1e-6) or clamp(sum, epsilon, INF)) before computing
the reciprocal and applying the energy compensation to lightingOutput.specular
so the original formula is preserved but never divides by zero.
In `@src/Features/VanillaFresnel.cpp`:
- Around line 22-25: VanillaFresnel::LoadSettings currently copies raw JSON into
settings; instead validate and sanitize each expected field: read each numeric
key from o_json (e.g., floats/ints used by the shader), clamp them to safe
min/max ranges, coerce boolean/toggle values to 0 or 1, apply sensible defaults
when keys are missing or wrong type, and then store the sanitized values back
into settings (or a sanitizedSettings struct) so that any later use (shader
constant upload code) only reads validated values; focus your changes inside
LoadSettings and reference the settings member and any shader-constant upload
path to ensure only the sanitized values are used.
- Around line 34-38: The ImGui::Checkbox calls currently reinterpret_cast uint
fields (settings.Enable, settings.EnableGGX, settings.EnableGGXOnGrass,
settings.EnableDynamicCubemapsConversion, settings.EnableEyeSpecialHandling) to
bool*, which is undefined behavior; replace each call with a temporary bool
(e.g., bool tmpEnable = settings.Enable != 0), pass &tmpEnable to
ImGui::Checkbox, and if ImGui reports a change assign back (settings.Enable =
tmpEnable ? 1u : 0u). Apply this pattern for each listed setting field in
VanillaFresnel.cpp instead of using reinterpret_cast.
In `@src/Features/VanillaFresnel.h`:
- Line 10: The GetFeatureModLink implementation in VanillaFresnel.h currently
returns MakeNexusModURL("999999"), which is a placeholder; update the virtual
inline std::string GetFeatureModLink() override to return the real Nexus mod ID
via MakeNexusModURL("<REAL_MOD_ID>") or return an empty string ("" ) until the
mod is published, ensuring the placeholder "999999" is removed.
---
Outside diff comments:
In `@src/FeatureBuffer.cpp`:
- Around line 39-56: The call to _GetFeatureBufferData in FeatureBuffer.cpp
passes vanillaFresnel.settings in the wrong position, breaking alignment with
HLSL's SharedData::FeatureData; update the argument order so that
globals::features::vanillaFresnel.settings is moved from before
linearLighting/terrainBlending/exponentialHeightFog to be the last argument
(i.e., placed after globals::features::exponentialHeightFog.settings) so the C++
packing matches the HLSL structure.
🪄 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: 1d7c6c7d-cc7d-414c-b9fd-f5de613bd912
📒 Files selected for processing (12)
features/Grass Lighting/Shaders/GrassLighting/GrassLighting.hlslifeatures/Vanilla Fresnel/Shaders/Features/VanillaFresnel.inipackage/Shaders/Common/LightingEval.hlslipackage/Shaders/Common/SharedData.hlslipackage/Shaders/Lighting.hlslpackage/Shaders/RunGrass.hlslsrc/Feature.cppsrc/FeatureBuffer.cppsrc/Features/VanillaFresnel.cppsrc/Features/VanillaFresnel.hsrc/Globals.cppsrc/Globals.h
|
✅ A pre-release build is available for this PR: |
Contributor
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 3095-3099: The code is conditionally suppressing the static envmap
specular term based on
SharedData::vanillaFresnelSettings.EnableDynamicCubemapsConversion even when the
DYNAMIC_CUBEMAPS permutation is not compiled, which removes specular but leaves
legacy diffuse; update the guard so the conversion-only env suppression is only
evaluated when DYNAMIC_CUBEMAPS is defined (in addition to the existing
VANILLA_FRESNEL/enableVanillaFresnel checks). Concretely, wrap the
EnableDynamicCubemapsConversion check with `#if` defined(DYNAMIC_CUBEMAPS) (or
combine DYNAMIC_CUBEMAPS into the existing preprocessor condition around
VANILLA_FRESNEL/enableVanillaFresnel) so the block that modifies specularColor
(and leaves indirectLobeWeights.diffuse unchanged) only runs when
DYNAMIC_CUBEMAPS is available; reference symbols: VANILLA_FRESNEL,
enableVanillaFresnel,
SharedData::vanillaFresnelSettings.EnableDynamicCubemapsConversion,
specularColor, indirectLobeWeights.diffuse, Color::IrradianceToLinear, envColor,
diffuseColor.
🪄 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: 842ebcdf-a4de-40fd-9361-534383a78ce8
📒 Files selected for processing (1)
package/Shaders/Lighting.hlsl
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Features/VanillaFresnel.h (1)
10-10:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the placeholder Nexus mod ID before merge.
Line 10 still uses a placeholder (
"999999"), which will produce a dead/misleading link in UI.Suggested fix
- virtual inline std::string GetFeatureModLink() override { return MakeNexusModURL("999999"); } + virtual inline std::string GetFeatureModLink() override { return ""; } // set real Nexus ID when published🤖 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/Features/VanillaFresnel.h` at line 10, GetFeatureModLink currently returns a hardcoded placeholder Nexus ID via MakeNexusModURL("999999"); update GetFeatureModLink to use the real Nexus mod ID (or a central constant/config value) instead of "999999" so the UI link is valid—locate the virtual inline method GetFeatureModLink in class VanillaFresnel and replace the placeholder string with the actual mod ID or a reference to a canonical MOD_ID/Config::NEXUS_MOD_ID symbol.
🧹 Nitpick comments (1)
src/Features/VanillaFresnel.h (1)
3-3: Add an issue reference keyword in the PR description.Since this PR introduces a new feature, add a tracker link like
Implements #<issue>orAddresses #<issue>for traceability.As per coding guidelines, "Issue References (if PR fixes bugs or implements features): Suggest adding appropriate GitHub keywords."
🤖 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/Features/VanillaFresnel.h` at line 3, Update the PR description to include a GitHub issue reference keyword for traceability (e.g., "Implements #<issue>" or "Addresses #<issue>") related to the new feature introduced by struct VanillaFresnel : public Feature so the addition of VanillaFresnel is linked to the corresponding tracker issue.
🤖 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/Features/VanillaFresnel.h`:
- Line 47: The member float pad in the VanillaFresnel struct/class is left
uninitialized; initialize pad to a deterministic value (e.g., 0.0f) to avoid
leaking indeterminate bytes during buffer uploads/serialization. Fix by
assigning pad = 0.0f where it's declared or by setting pad in the VanillaFresnel
constructor (or initializer list) so the member is always initialized.
---
Duplicate comments:
In `@src/Features/VanillaFresnel.h`:
- Line 10: GetFeatureModLink currently returns a hardcoded placeholder Nexus ID
via MakeNexusModURL("999999"); update GetFeatureModLink to use the real Nexus
mod ID (or a central constant/config value) instead of "999999" so the UI link
is valid—locate the virtual inline method GetFeatureModLink in class
VanillaFresnel and replace the placeholder string with the actual mod ID or a
reference to a canonical MOD_ID/Config::NEXUS_MOD_ID symbol.
---
Nitpick comments:
In `@src/Features/VanillaFresnel.h`:
- Line 3: Update the PR description to include a GitHub issue reference keyword
for traceability (e.g., "Implements #<issue>" or "Addresses #<issue>") related
to the new feature introduced by struct VanillaFresnel : public Feature so the
addition of VanillaFresnel is linked to the corresponding tracker issue.
🪄 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: 6a9f3136-3332-4a8f-b952-8ef942d5a17d
📒 Files selected for processing (2)
features/Vanilla Fresnel/COREsrc/Features/VanillaFresnel.h
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new "Vanilla Fresnel" feature, providing advanced Fresnel and GGX-based physically-based rendering (PBR) specular reflection options, with extensive configurability and integration throughout the shader pipeline. The changes add new settings, update data structures, and modify lighting calculations in both general and grass-specific shaders to support these features. Additionally, the feature is integrated into the engine's feature management and configuration systems.
Major new feature: Vanilla Fresnel
VanillaFresnelfeature with configurable settings (GGX specular, dynamic cubemap conversion, eye handling, etc.), UI controls, and serialization support (src/Features/VanillaFresnel.cpp,features/Vanilla Fresnel/Shaders/Features/VanillaFresnel.ini). [1] [2]Shader pipeline and lighting model updates
SharedData::VanillaFresnelSettings), and updated the buffer to include these settings (package/Shaders/Common/SharedData.hlsli,src/FeatureBuffer.cpp). [1] [2] [3]package/Shaders/Lighting.hlsl,package/Shaders/Common/LightingEval.hlsli). [1] [2] [3] [4] [5] [6] [7]package/Shaders/Common/LightingEval.hlsli).Grass lighting improvements
features/Grass Lighting/Shaders/GrassLighting/GrassLighting.hlsli,package/Shaders/RunGrass.hlsl). [1] [2] [3] [4] [5] [6]Engine integration
src/Feature.cpp,src/FeatureBuffer.cpp). [1] [2] [3]These changes collectively enable a more physically-accurate and configurable specular reflection model, improving visual fidelity and user control over rendering behavior.
Summary by CodeRabbit
New Features
Bug Fixes / Behavior