fix(pbr): use scrap heap allocation

[alandtse]

PBR material Make() was using operator new (via TES_HEAP_REDEFINE_NEW), which allocates from the global heap with RE::malloc. The game expects materials to be allocated from the Thread ScrapHeap with align=8. This mismatch causes heap corruption when materials are later freed via scrap heap deallocation paths.

Switch to scrapHeap->Allocate + std::construct_at, matching the pattern used by CommonLib's BSLightingShaderMaterialBase::CreateMaterial

Ref: #1895,
closes #1654

Summary by CodeRabbit

• Refactor
  • Optimized memory allocation and initialization for shader materials, improving runtime memory efficiency, stability, and startup reliability for rendering. Creation paths unified to reduce inconsistencies, which can result in fewer rendering glitches and more consistent performance across scenes.

[coderabbitai]

📝 Walkthrough

Walkthrough

Modifies the Make() factory methods in two BSLightingShaderMaterialPBR classes to allocate from the thread scrap heap instead of using operator new, with null-checking, explicit zeroing via std::memset, and object construction via std::construct_at.

Changes

Cohort / File(s)	Summary
Shader Material Memory Allocation src/TruePBR/BSLightingShaderMaterialPBR.cpp, src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp	Replaced direct operator new allocation with memoryManager->GetThreadScrapHeap()->AllocateAligned(8, size); added null-check, std::memset zero-initialization, and std::construct_at to default-construct the object; Create() now delegates to Make() where applicable.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

• fix: heap corruption in BSLightingShaderMaterialPBR allocation (kMenuScreen/PBR path) #1910 — Matches the allocator/alignment change in BSLightingShaderMaterialPBR::Make(), addressing the allocator/alignment mismatch described.
• #1654 — Heap corruption: changes align allocation/deallocation to scrap-heap usage and explicit construction/zeroing, which may prevent the heap corruption observed during destruction.

Suggested reviewers

• doodlum
• jiayev

Poem

🐰 I hopped through bytes where shadows play,
Replaced raw new with tidier clay,
Zeroed the nest, built each part—
No more crumbs to break the heart. 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 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 (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(pbr): use scrap heap allocation' directly and clearly summarizes the main change: switching PBR material allocation from operator new to scrap heap allocation.
Linked Issues check	✅ Passed	The PR changes align with issue #1654's core requirement: allocating materials from Thread ScrapHeap with 8-byte alignment to eliminate heap corruption caused by mismatched allocation/deallocation paths.
Out of Scope Changes check	✅ Passed	All changes are scoped to PBR material allocation in two files, directly addressing the heap corruption issue without introducing unrelated modifications.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

No actionable suggestions for changed features.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/TruePBR/BSLightingShaderMaterialPBR.cpp`:
- Around line 12-18: Add a defensive null check for the scrap heap before
dereferencing it in the allocation block that creates
BSLightingShaderMaterialPBR: when calling
RE::MemoryManager::GetSingleton()->GetThreadScrapHeap() (scrapHeap) verify
scrapHeap is non-null before calling scrapHeap->Allocate and only proceed to
memset/construct_at if allocation succeeded; if scrapHeap is null, return
nullptr (or fall back to a safe allocation path such as operator new) so
BSLightingShaderMaterialPBR creation cannot dereference a null scrapHeap (update
the same pattern used in BSLightingShaderMaterialPBR and mirror in
BSLightingShaderMaterialPBRLandscape.cpp if desired).

---

Duplicate comments:
In `@src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp`:
- Around line 18-24: In BSLightingShaderMaterialPBRLandscape::Make (the block
allocating via scrapHeap), add the same null guard used in
BSLightingShaderMaterialPBR::Make: check if scrapHeap is null immediately after
calling RE::MemoryManager::GetSingleton()->GetThreadScrapHeap() and return
nullptr (or handle failure) before dereferencing it; keep the existing
allocation, memset, and std::construct_at logic unchanged once scrapHeap is
confirmed non-null.

[github-actions]

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

[doodlum]

use globals for singleton

[coderabbitai]

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@src/TruePBR/BSLightingShaderMaterialPBR.cpp`:
- Around line 12-13: The code dereferences scrapHeap returned by
globals::game::memoryManager->GetThreadScrapHeap() before a null check; update
the allocation logic in BSLightingShaderMaterialPBR creation to first check if
scrapHeap is non-null, and only call
scrapHeap->Allocate(sizeof(BSLightingShaderMaterialPBR), 8) when it is valid,
otherwise fallback to an alternative allocation (e.g., use operator new or
another allocator) and then proceed with the existing `if (material)` handling;
reference GetThreadScrapHeap, scrapHeap, Allocate and
BSLightingShaderMaterialPBR to locate and modify the allocation block.

In `@src/TruePBR/BSLightingShaderMaterialPBRLandscape.cpp`:
- Around line 18-19: The code dereferences
globals::game::memoryManager->GetThreadScrapHeap() into scrapHeap and calls
scrapHeap->Allocate(...) without a null check; add the same defensive guard used
in the PBR variant so that if scrapHeap is null you fall back to a normal
allocation path and construct the object there instead of calling Allocate.
Specifically, in the block that creates the BSLightingShaderMaterialPBRLandscape
instance (the lines using scrapHeap and the static_cast to
BSLightingShaderMaterialPBRLandscape), check if scrapHeap is non-null before
calling scrapHeap->Allocate; if it is null, allocate raw memory via the usual
fallback (e.g., operator new/malloc) and use placement/new construction
consistent with how BSLightingShaderMaterialPBR handles the case.