Skip to content

fix(ibl): crash when dynamic cubemaps disabled#1439

Merged
doodlum merged 2 commits into
community-shaders:devfrom
jiayev:IBL
Sep 6, 2025
Merged

fix(ibl): crash when dynamic cubemaps disabled#1439
doodlum merged 2 commits into
community-shaders:devfrom
jiayev:IBL

Conversation

@jiayev
Copy link
Copy Markdown
Collaborator

@jiayev jiayev commented Sep 1, 2025

This pull request updates the IBL prepass logic to better handle cases where dynamic cubemaps are not loaded, preventing potential null dereferences in shader resource views.

Resource management improvements:

  • In src/Features/IBL.cpp, the assignment of envTexture and envReflectionsTexture is changed from const auto& to auto& to allow modification if needed.
  • When constructing the srvs array, the shader resource views for environment textures are now conditionally set to nullptr if dynamicCubemaps.loaded is false, improving robustness against missing resources.

Summary by CodeRabbit

  • Bug Fixes

    • Environment textures are now only bound when dynamic cubemaps are available, preventing flickering or incorrect reflections during loading.
  • Performance

    • Avoids unnecessary binding of environment resources when not loaded, reducing GPU work and improving stability during startup and scene changes.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Sep 1, 2025

Walkthrough

The prepass in src/Features/IBL.cpp now makes local references to environment textures mutable and conditionally binds envTexture and envReflectionsTexture SRVs only when dynamicCubemaps.loaded is true; otherwise those SRV slots receive nullptr. No public interfaces changed.

Changes

Cohort / File(s) Summary
IBL prepass SRV binding
src/Features/IBL.cpp
Change envTexture/envReflectionsTexture from const auto& to auto&. Initialize SRV array entries for environment textures to (dynamicCubemaps.loaded && envTexture) ? envTexture->srv.get() : nullptr and (dynamicCubemaps.loaded && envReflectionsTexture) ? envReflectionsTexture->srv.get() : nullptr; other prepass logic unchanged.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Prepass as IBL::Prepass
  participant Env as Environment Textures
  participant CS as Compute Shader

  Prepass->>Env: Acquire envTexture, envReflectionsTexture (auto& mutable refs)
  alt dynamicCubemaps.loaded == true
    Prepass->>CS: Bind SRVs [..., envTexture.srv, envReflectionsTexture.srv, ...]
    note right of CS #DFF2E1: Env textures available for sampling
  else
    Prepass->>CS: Bind SRVs [..., null, null, ...]
    note right of CS #FEEFEF: Env sampling disabled (null SRVs)
  end
  Prepass->>CS: Dispatch compute shader
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • alandtse
  • doodlum

Poem

I hop through code with careful paws,
I bind the skies only when they’re gauged,
If cubemaps load, the stars applause,
If not, I leave those slots unstaged. 🐇✨


📜 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 5173d0e and 39addb7.

📒 Files selected for processing (1)
  • src/Features/IBL.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/Features/IBL.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
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 1, 2025

Using provided base ref: 256a385
Using base ref: 256a385
Base commit date: 2025-08-29T00:02:23-07:00 (Friday, August 29, 2025 12:02 AM)
No actionable suggestions for changed features.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/Features/IBL.cpp (1)

72-73: Non-const refs seem unnecessary here

These locals aren’t mutated. Unless something below actually writes to them, keep them const for clarity, or justify the mutability in a comment.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

💡 Knowledge Base configuration:

  • MCP integration is disabled by default for public repositories
  • Jira integration is disabled by default for public repositories
  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 256a385 and 5173d0e.

📒 Files selected for processing (1)
  • src/Features/IBL.cpp (1 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/Features/IBL.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/Features/IBL.cpp
🧠 Learnings (2)
📓 Common learnings
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-18T15:21:03.641Z
Learning: In the skyrim-community-shaders rendering pipeline, materials with alpha < 1 or alpha blending enabled are rendered in non-deferred mode rather than deferred mode. This means issues with dynamic cubemaps on transparent materials are actually non-deferred rendering issues.
Learnt from: alandtse
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-07-05T05:20:45.823Z
Learning: In the skyrim-community-shaders repository, file deletion error handling improvements that replace existence checks and try-catch blocks with std::filesystem::remove error-code-based approaches are considered bug fixes rather than refactoring, as they address inadequate error handling and misleading log messages.
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.
📚 Learning: 2025-08-03T18:37:19.690Z
Learnt from: jiayev
PR: doodlum/skyrim-community-shaders#0
File: :0-0
Timestamp: 2025-08-03T18:37:19.690Z
Learning: ISReflectionsRayTracing.hlsl and ISWorldMap.hlsl in the skyrim-community-shaders repository are image-space post-processing shaders that perform color sampling and blending operations that need proper linear color space handling for the linear lighting system. ISReflectionsRayTracing handles screen-space reflections and should use conditional Color::IrradianceToLinear/Gamma conversions similar to ISCompositeLensFlareVolumetricLighting.hlsl. ISWorldMap performs 7x7 color accumulation that should be done in linear space similar to the pattern used in ISSAOComposite.hlsl.

Applied to files:

  • src/Features/IBL.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 (1)
src/Features/IBL.cpp (1)

75-75: Confirm shader recompilation on dynamicCubemaps.loaded toggle

I couldn’t find where GetDiffuseIBLCS() is recompiled or ClearShaderCache() is called when dynamicCubemaps.loaded changes. Please verify that toggling this flag triggers a shader cache clear or recompile so the bound SRVs always match shader expectations.

Comment thread src/Features/IBL.cpp Outdated
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Sep 1, 2025

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

@alandtse
Copy link
Copy Markdown
Collaborator

alandtse commented Sep 2, 2025

Please review all AI comments and mark as resolved once done.

@doodlum doodlum merged commit d261378 into community-shaders:dev Sep 6, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants