fix: fix pbr heap crash by doodlum · Pull Request #1706 · community-shaders/skyrim-community-shaders

doodlum · 2026-01-13T15:45:17Z

Summary by CodeRabbit

Refactor
- Changed internal allocation for shader material components to use per-thread scrap heaps, improving runtime allocation efficiency and reducing overhead.
Bug Fixes / Stability
- Added safer placement-deallocation handling for shader materials to reduce leak risk and improve stability during resource cleanup.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

coderabbitai · 2026-01-13T15:45:25Z

📝 Walkthrough

Walkthrough

Two shader material classes now allocate instances from the per-thread scrap heap via RE::MemoryManager and construct them in-place using placement new; corresponding headers add placement-delete operators calling RE::free(). Public Create() signatures and external behavior are unchanged.

Changes

Cohort / File(s)	Summary
Thread scrap heap allocation `src/TruePBR/BSLightingShaderMaterialPBR.cpp`, `src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp`	`Make()` now obtains the thread scrap heap, allocates aligned memory from it, and constructs the object in-place (placement new). `Create()` still forwards to `Make()`.
Placement delete operators added `src/TruePBR/BSLightingShaderMaterialPBR.h`, `src/TruePBR/BSLightingShaderMaterialPBRLandscape.h`	Added `static void operator delete(void* ptr, void*) noexcept` implementations that call `RE::free(ptr)` to pair with the placement-new/allocation changes.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐇 I hop where threads and scrapped heaps meet,

I shape new forms in memory neat,
Constructed snug with a careful spin,
When my work is done, RE::free lets me bin,
A tiny rabbit's build — precise and fleet.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main objective: fixing a PBR heap crash by implementing proper memory allocation and placement delete operators.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

📝 Generate docstrings

📜 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 2288672 and 608c152.

📒 Files selected for processing (2)

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp
src/TruePBR/BSLightingShaderMaterialPBR.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/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp
src/TruePBR/BSLightingShaderMaterialPBR.cpp

**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:

feat(vr): add cross-eye sampling

fix(water): resolve flowmap bug

docs: update shader documentation

Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:

"Fixes feat: increased portal strict light limit #123" or "Closes feat: increased portal strict light limit #123" for bug fixes

"Implements feat: increased portal strict light limit #123" or "Addresses feat: increased portal strict light limit #123" for features

"Related to feat: increased portal strict light limit #123" for partial implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp
src/TruePBR/BSLightingShaderMaterialPBR.cpp

🧠 Learnings (1)

📓 Common learnings

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.

🔇 Additional comments (2)

src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp (1)

16-21: Allocation pattern is consistent with BSLightingShaderMaterialPBR::Make(), using the same thread scrap heap approach with proper placement new/delete semantics. Both material classes follow the game engine's expected memory management.
src/TruePBR/BSLightingShaderMaterialPBR.cpp (1)
10-15: Heap allocation fix looks correct.

The change to allocate from the thread scrap heap matches the game engine's expected allocation pattern for shader materials, which should resolve the heap crash. The 8-byte alignment is appropriate, and the static singleton access is thread-safe. The placement delete operator is already present in the header (line 53), so exception safety during construction is properly handled.
Minor consideration: defensive null check

If GetThreadScrapHeap() could ever return nullptr in edge cases, this would crash. Consider whether a null check is warranted:
 BSLightingShaderMaterialPBR* BSLightingShaderMaterialPBR::Make()
 {
 	static auto memoryManager = RE::MemoryManager::GetSingleton();
-	auto memory = static_cast<BSLightingShaderMaterialPBR*>(memoryManager->GetThreadScrapHeap()->Allocate(sizeof(BSLightingShaderMaterialPBR), 8));
+	auto* heap = memoryManager->GetThreadScrapHeap();
+	if (!heap) {
+		return nullptr;
+	}
+	auto* memory = heap->Allocate(sizeof(BSLightingShaderMaterialPBR), 8);
 	return new (memory) BSLightingShaderMaterialPBR();
 }

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 · 2026-01-13T15:46:23Z

Using provided base ref: f8aa7a2
Using base ref: f8aa7a2
Base commit date: 2026-01-13T15:11:17Z (Tuesday, January 13, 2026 03:11 PM)
No actionable suggestions for changed features.

coderabbitai

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @src/TruePBR/BSLightingShaderMaterialPBR.cpp:
- Around line 10-15: BSLightingShaderMaterialPBR::Make currently allocates raw
memory from a static scrapHeap and never runs the constructor, leaving members
like pbrFlags, coatRoughness, fuzzColor and texture smart pointers uninitialized
and also incorrectly caches a thread-local heap; change it to obtain the heap
per-call (remove the static scrapHeap) and perform placement-new on the
allocated memory (new (mem) BSLightingShaderMaterialPBR()) so the constructor
runs and initializes members, and add #include <new> to enable placement new if
not already included.

In @src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp:
- Around line 16-21: The Make() function currently allocates raw memory from a
cached static scrapHeap and never invokes the constructor; fix it by retrieving
the thread-local heap on each call (call
RE::MemoryManager::GetSingleton()->GetThreadScrapHeap() inside Make()—do NOT
cache it in a static), allocate the required size from that per-call heap, and
then construct the object with placement new so the constructor of
BSLightingShaderMaterialPBRLandscape runs and initializes members (do the same
change for BSLightingShaderMaterialPBR::Make()). Ensure you replace the static
scrapHeap with a local variable and perform placement-new on the allocated
memory before returning the pointer.

📜 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 f8aa7a2 and 8887bf3.

📒 Files selected for processing (2)

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.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/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:

feat(vr): add cross-eye sampling

fix(water): resolve flowmap bug

docs: update shader documentation

Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:

"Fixes #123" or "Closes #123" for bug fixes

"Implements #123" or "Addresses #123" for features

"Related to #123" for partial implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🧠 Learnings (1)

📓 Common learnings

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.

⏰ 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). (2)

GitHub Check: Run Shader Unit Tests
GitHub Check: Build plugin and addons

…ty-shaders into pbr-fixes

coderabbitai

Actionable comments posted: 2

🤖 Fix all issues with AI agents

In @src/TruePBR/BSLightingShaderMaterialPBR.cpp:
- Around line 12-14: Do not cache the MemoryManager singleton as a static; call
RE::MemoryManager::GetSingleton() each time and use its
GetThreadScrapHeap()->Allocate(...) result for placement new, and add a null
check on the returned pointer before calling placement new on
BSLightingShaderMaterialPBR; if Allocate() returns nullptr, handle the failure
(e.g., log via your logger and return nullptr or propagate an error) instead of
dereferencing a null pointer.

📜 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 8887bf3 and 476e0b6.

📒 Files selected for processing (4)

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBR.h
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.h

🚧 Files skipped from review as they are similar to previous changes (1)

src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.h
src/TruePBR/BSLightingShaderMaterialPBR.h

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/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.h
src/TruePBR/BSLightingShaderMaterialPBR.h

**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:

feat(vr): add cross-eye sampling

fix(water): resolve flowmap bug

docs: update shader documentation

Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:

"Fixes #123" or "Closes #123" for bug fixes

"Implements #123" or "Addresses #123" for features

"Related to #123" for partial implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.h
src/TruePBR/BSLightingShaderMaterialPBR.h

🧠 Learnings (1)

📓 Common learnings

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.

⏰ 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/TruePBR/BSLightingShaderMaterialPBR.h (1)

52-53: Implementation is consistent with the Landscape variant.

The placement delete operator matches the pattern used in BSLightingShaderMaterialPBRLandscape. The same verification regarding RE::free() compatibility with thread scrap heap allocation applies here.

github-actions · 2026-01-13T21:21:36Z

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

coderabbitai

Actionable comments posted: 1

🤖 Fix all issues with AI agents

In @src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp:
- Around line 18-21: The code currently assigns to raw allocated memory with
"*memory = {}" which invokes assignment on unconstructed memory; instead, call
the constructor in-place using placement new to construct a
BSLightingShaderMaterialPBRLandscape at the pointer returned by
memoryManager->GetThreadScrapHeap()->Allocate(...) so the object's non-trivial
constructor and destructor run (replace the assignment with placement new for
BSLightingShaderMaterialPBRLandscape at variable memory and then return that
pointer).

📜 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 476e0b6 and 2288672.

📒 Files selected for processing (2)

src/TruePBR/BSLightingShaderMaterialPBR.cpp
src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🚧 Files skipped from review as they are similar to previous changes (1)

src/TruePBR/BSLightingShaderMaterialPBR.cpp

🧰 Additional context used

📓 Path-based instructions (3)

**/*.{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/TruePBR/BSLightingShaderMaterialPBRLandscape.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/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

**/*

⚙️ CodeRabbit configuration file

**/*: When reviewing PRs, please provide suggestions for:

Conventional Commit Titles (if not following https://www.conventionalcommits.org/ or
if the existing title does not describe the code changes):
Format: type(scope): description
Length: 50 characters limit for title, 72 for body
Style: lowercase description, no ending period
Examples:

feat(vr): add cross-eye sampling

fix(water): resolve flowmap bug

docs: update shader documentation

Issue References (if PR fixes bugs or implements features):
Suggest adding appropriate GitHub keywords:

"Fixes #123" or "Closes #123" for bug fixes

"Implements #123" or "Addresses #123" for features

"Related to #123" for partial implementations

Otherwise, use your standard review approach focusing on code quality.

Files:

src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

🧠 Learnings (1)

📓 Common learnings

Learnt from: alandtse
Repo: doodlum/skyrim-community-shaders PR: 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.

⏰ 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

SimplisticMind · 2026-01-14T15:44:30Z

scrapheap is functionally identical to memoryallocator except for size and alignment minimums (refer to: https://github.com/aers/EngineFixesSkyrim64/blob/master/src/memory/scrapheap.h#L11 )
new operator is overridden and uses memoryallocator already (refer to: https://github.com/alandtse/CommonLibVR/blob/vr/include/RE/B/BSIntrusiveRefCounted.h#L25 and https://github.com/alandtse/CommonLibVR/blob/vr/include/RE/M/MemoryManager.h#L204 )

fix: fix pbr heap crash

0ef60ff

chore: update pbr fixes branch to dev (#1707)

8887bf3

coderabbitai Bot reviewed Jan 13, 2026

View reviewed changes

Comment thread src/TruePBR/BSLightingShaderMaterialPBR.cpp

Comment thread src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp

doodlum added 5 commits January 13, 2026 16:00

fix: fix threading

bd6d70d

Merge branch 'pbr-fixes' of https://github.com/doodlum/skyrim-communi…

f844292

…ty-shaders into pbr-fixes

fix: fix more nonsense

2813212

fix: use actual alignment

d696b0f

fix: fix compilation warning

476e0b6

coderabbitai Bot reviewed Jan 13, 2026

View reviewed changes

Comment thread src/TruePBR/BSLightingShaderMaterialPBR.cpp

Comment thread src/TruePBR/BSLightingShaderMaterialPBRLandscape.h

fix: fix attempt

2288672

coderabbitai Bot reviewed Jan 13, 2026

View reviewed changes

Comment thread src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp Outdated

fix: fix nonsense

608c152

doodlum closed this Jan 17, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fix pbr heap crash#1706

fix: fix pbr heap crash#1706
doodlum wants to merge 9 commits into
devfrom
pbr-fixes

doodlum commented Jan 13, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jan 13, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Uh oh!

github-actions Bot commented Jan 13, 2026

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jan 13, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

SimplisticMind commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

doodlum commented Jan 13, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Poem

Uh oh!

github-actions Bot commented Jan 13, 2026

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jan 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

SimplisticMind commented Jan 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

doodlum commented Jan 13, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jan 13, 2026 •

edited

Loading

github-actions Bot commented Jan 13, 2026 •

edited

Loading