Improve deferred decal rendering performance - #1
Conversation
📝 WalkthroughWalkthroughThe change adds receiver-aware MSFS decal projection, WebGPU aircraft G-buffer resources, deferred-lighting material writers, deferred render-pass orchestration, forward fallback, and cockpit-dependent activation. Tests cover projection, materials, G-buffer resources, and pass selection. ChangesMSFS deferred rendering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AnimationLoop
participant MsfsRenderPasses
participant AircraftGBuffer
participant SceneMaterials
AnimationLoop->>MsfsRenderPasses: setDeferredEnabled(cockpitViewInactive)
MsfsRenderPasses->>SceneMaterials: collect visible decals and receivers
MsfsRenderPasses->>AircraftGBuffer: render G-buffer passes
MsfsRenderPasses->>SceneMaterials: render deferred resolve batches
MsfsRenderPasses-->>AnimationLoop: restore rendering state
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29b6f853e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (!isDecalProjectionVisible(decalPrimitive.mesh, triangleIndex, occluderIndex)) { | ||
| continue | ||
| } |
There was a problem hiding this comment.
When a same-parent decal has even one vertex closer to a descendant mesh than to the candidate receiver, isDecalProjectionVisible rejects the entire primitive, including portions that are unobstructed. Ordered or component-only decals then disappear because the forward path hides unprojected materials, while unordered color decals remain as detached shells. This all-or-nothing geometric heuristic should be replaced with per-region clipping or an authoritative receiver relationship.
AGENTS.md reference: AGENTS.md:L5-L5
Useful? React with 👍 / 👎.
| isBlendGBufferMaterial && buildGBufferWriter | ||
| ? 1 |
There was a problem hiding this comment.
For a projected blend-G-buffer material with a writer, this changes the source material's normal scale to full strength instead of applying blendFactors.normal. That source material is still used whenever deferred rendering is unavailable and is explicitly used after src/main.ts disables deferred rendering in cockpit view, so decals with a zero or fractional normal factor incorrectly apply their full normal map in those contexts. Keep the forward material scaled by the authored factor and give only the writer an unscaled copy.
Useful? React with 👍 / 👎.
| if (gBufferWriter != null) { | ||
| outputMaterial.userData.msfsGBufferWriter = gBufferWriter |
There was a problem hiding this comment.
When an exterior component is replaced or reloaded, disposeObjectResources disposes only each mesh's top-level material; Three.js does not recursively dispose a second Material stored in userData. Consequently every compiled writer assigned here survives exterior LOD/settings reloads without a disposal event, retaining its renderer resources and texture references. The component teardown path should explicitly dispose and clear this attached writer.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
src/msfs/gltf/normalizeMsfsMaterials.test.ts (4)
140-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFail loudly on multi-component swizzles.
Line 145 uses
'xyzw'.indexOf(node.components). For a multi-component swizzle such as'xyz'or'rgb',indexOfreturns-1and Line 146 returnsundefined. The helper then producesNaNdownstream instead of an error.The current assertions never reach that path, so this is a latent trap only. Add an explicit check so a future graph change fails with a clear message.
♻️ Proposed change
const index = 'xyzw'.indexOf(node.components) + if (index < 0) { + throw new Error(`Unsupported swizzle components: ${node.components}`) + } return source[index]🤖 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/msfs/gltf/normalizeMsfsMaterials.test.ts` around lines 140 - 147, Update the split-node handling around evaluateCoverageNode so multi-component or otherwise unsupported node.components values are rejected explicitly before indexing; validate that the swizzle resolves to a single valid xyzw component, and throw a clear error instead of returning undefined and allowing NaN downstream.
597-627: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the case where the receiver carries no skin influences.
This test gives the receiver both
skinIndexandskinWeight, so it exercises only thehasProjectedSkin === truepath inrebuildConformedDecalGeometry.When the receiver has no skin influences, that flag stays false and the decal's own
skinIndexruns through the generic barycentric interpolation loop. I raised that defect onsrc/msfs/gltf/normalizeMsfsMaterials.tsat Line 1666. A test with an unskinned receiver and a skinned decal would pin the fix.🤖 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/msfs/gltf/normalizeMsfsMaterials.test.ts` around lines 597 - 627, The test around normalizeMsfsMaterials should also cover an unskinned receiver with a skinned decal. Add a case where the receiver lacks skinIndex and skinWeight while the decal has those attributes, then assert the decal does not retain or generically interpolate its skin influences after rebuildConformedDecalGeometry. Preserve the existing projected normal and tangent assertions.
189-193: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAssert values directly instead of wrapping comparisons in
toBe(true).Line 189, Line 193, Line 252, Line 298, Line 325, Line 350 and Line 377 all compute a boolean and then assert
toBe(true). On failure Bun reports onlyexpected true, received false. The actual coordinate or count is lost, which makes projection regressions slow to diagnose.
bun:testprovidestoBeCloseToandtoBeLessThan, which report both operands.♻️ Example for Line 189 and Line 298
- expect(position.count === 3).toBe(true) + expect(position.count).toBe(3)- expect(Math.abs(decal.geometry.getAttribute('position').getZ(0) + 0.1) < 1e-6).toBe(true) + expect(decal.geometry.getAttribute('position').getZ(0)).toBeCloseTo(-0.1, 6)Also applies to: 251-256, 277-278, 298-298, 325-325, 350-350, 373-378
🤖 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/msfs/gltf/normalizeMsfsMaterials.test.ts` around lines 189 - 193, Update the assertions in the affected normalizeMsfsMaterials tests to assert values directly rather than comparing boolean expressions with toBe(true). Replace count equality checks with direct equality assertions and tolerance checks with toBeCloseTo or toBeLessThan, preserving the existing expected values and thresholds so failures report the actual operands.
404-432: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for the independent-ORM rejection branch.
createMsfsGBufferWriterreturnsnullwhenblendFactors.roughnessdiffers fromblendFactors.metallicorblendFactors.occlusion. That branch disposes the writer and forcessrc/rendering/createMsfsRenderPasses.tsinto a fallback path.No test covers it. A decal material with, for example,
roughnessBlendFactor: 0.5andmetallicBlendFactor: 1should yieldgetMsfsGBufferWriter(...) === null.Do you want me to draft that test case?
🤖 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/msfs/gltf/normalizeMsfsMaterials.test.ts` around lines 404 - 432, The existing G-buffer writer test lacks coverage for independent ORM blend factors. Extend the test in the `builds G-buffer writers only for node-compatible materials` case, or add a focused case, using a decal material whose `ASOBO_material_blend_gbuffer` sets different roughness and metallic factors (for example 0.5 and 1), and assert that `getMsfsGBufferWriter(...)` returns null.src/msfs/gltf/normalizeMsfsMaterials.ts (1)
1093-1105: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClone
projectionNormalbefore you mutate it.
projectionNormalsis typedReadonlyMap<number, Vector3>, but Line 1095 and Line 1105 mutate the storedVector3in place. Line 1105 converts it intoyAxis. TodaybuildBlendGBufferProjectionNormalsstores a distinct clone per triangle, so no corruption occurs. If that helper is ever changed to share one instance across a connected component, every triangle after the first would read a corrupted normal. Clone at the read site to remove the coupling.♻️ Proposed change
- const projectionNormal = projectionNormals.get(triangleOffset / 3) ?? sourceNormal + const projectionNormal = (projectionNormals.get(triangleOffset / 3) ?? sourceNormal).clone()🤖 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/msfs/gltf/normalizeMsfsMaterials.ts` around lines 1093 - 1105, Clone the normal retrieved from projectionNormals before any mutation in the triangle projection flow. Update the projectionNormal initialization near sourceNormal so both mapped and fallback normals are copied, preserving the existing negation and cross-product behavior without mutating stored Vector3 instances.src/rendering/createMsfsRenderPasses.ts (5)
633-652: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider rejecting
lightMapalongsideaoMap.The list rejects
aoMap,alphaMap,bumpMap, and the other spatially varying inputs, because projected geometry does not carry the receiver's original UVs.lightMapbelongs to the same category and is not checked. A receiver with a bakedlightMapwould pass this predicate and then write an incorrect G-buffer value from projected geometry.♻️ Proposed change
bumpMap?: unknown colorNode?: unknown displacementMap?: unknown emissiveMap?: unknown emissiveNode?: unknown + lightMap?: unknown map?: unknownsource.displacementMap == null && source.emissiveMap == null && + source.lightMap == null && source.map == null &&🤖 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/rendering/createMsfsRenderPasses.ts` around lines 633 - 652, Update the material eligibility predicate to reject materials with a non-null lightMap, alongside aoMap and the other UV-dependent map checks. Keep the existing acceptance behavior unchanged for materials without lightMap.
449-453: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
configureDeferredDecalWriterDepthTestonly ever setsdepthTest = true.The helper at Lines 757-770 assigns
trueunconditionally. It is called at Line 449 before the decal G-buffer pass and again at Line 488 in thefinallyblock. Both calls produce the same state, so neither call changes anything that the other does not.If a pass was meant to disable depth testing on the writers, that branch is missing. If not, remove one call and rename the helper to describe the invariant, or drop it and set
depthTestonce when the writer materials are created.Also applies to: 757-770
🤖 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/rendering/createMsfsRenderPasses.ts` around lines 449 - 453, Resolve the redundant configureDeferredDecalWriterDepthTest calls around the decal G-buffer pass and its finally block. Since the helper only assigns depthTest = true, either add the missing disable branch if the pass requires it, or remove the duplicate call and rename the helper to reflect the invariant, preferably setting writer material depth testing once during creation.
397-403: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoidable per-frame work remains in the deferred render path. The PR goal is to remove work from the hot path, but
renderstill allocates scratch objects on entry and repeats map lookups in a helper it calls six times per frame.
src/rendering/createMsfsRenderPasses.ts#L397-L403: hoist theColorscratch to module scope next todrawingBufferSize, and hoist theoriginalMaterialStatemap into the closure with a.clear()at the start ofrender.src/rendering/createMsfsRenderPasses.ts#L734-L743: iteratesourceMaterialsentries once instead of calling.get(mesh)twice per mesh, and avoid the per-call array allocation for array materials.🤖 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/rendering/createMsfsRenderPasses.ts` around lines 397 - 403, The deferred render path should avoid per-frame scratch allocations and redundant material lookups. In src/rendering/createMsfsRenderPasses.ts:397-403, hoist the Color scratch object to module scope beside drawingBufferSize, move originalMaterialState into the render closure, and clear it at the start of render. In src/rendering/createMsfsRenderPasses.ts:734-743, update the material-state helper to iterate sourceMaterials entries once, reuse each entry’s material value, and handle array materials without allocating a new array on every call.
60-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
selectMsfsDecalRenderPathincreateMsfsRenderPasses, or remove it.The helper has no production call site. Only
src/rendering/createMsfsRenderPasses.test.tsuses it. The factory keeps a separate inline decision, so the tested rule can diverge from production behavior.🤖 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/rendering/createMsfsRenderPasses.ts` around lines 60 - 71, Update createMsfsRenderPasses to use selectMsfsDecalRenderPath for its decal render-path decision, removing the duplicate inline logic and keeping production behavior aligned with the tested helper; alternatively remove the unused helper and update its tests if the factory’s inline decision is the intended source of truth.
539-563: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid recomputing transforms in
canShareResolveBatch.
syncResolveBatchinvokes this predicate for every selected batch on every render. The predicate updates both meshes and compares full matrices, thensyncResolveBatchupdates the source again. Update transforms once in the caller and keep the predicate side-effect-free.🤖 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/rendering/createMsfsRenderPasses.ts` around lines 539 - 563, Update syncResolveBatch to call updateMatrix() once for each candidate mesh before invoking canShareResolveBatch, then remove the transform updates from canShareResolveBatch. Keep canShareResolveBatch side-effect-free while retaining its existing matrix and layer comparisons.src/rendering/createMsfsRenderPasses.test.ts (2)
88-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCover the boolean and numeric rejection branches, and match the cast style used elsewhere.
canWriteReceiverGBufferFromProjectedGeometryhas two structurally different groups of checks:== nullchecks on maps and nodes, and value checks ontransparent,alphaHash,alphaTest, andvertexColors. This test covers only the first group.Line 94 also uses
as any, whilesrc/rendering/createAircraftGBuffer.test.tsLine 27 usesas neverfor the same purpose.💚 Proposed change
- ;(material as any).normalNode = {} + ;(material as never as { normalNode: unknown }).normalNode = {} expect(canWriteReceiverGBufferFromProjectedGeometry(material)).toBe(false) + ;(material as never as { normalNode: unknown }).normalNode = null + material.transparent = true + expect(canWriteReceiverGBufferFromProjectedGeometry(material)).toBe(false) + material.transparent = false + material.alphaTest = 0.5 + expect(canWriteReceiverGBufferFromProjectedGeometry(material)).toBe(false) + material.alphaTest = 0 + material.vertexColors = true + expect(canWriteReceiverGBufferFromProjectedGeometry(material)).toBe(false) })🤖 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/rendering/createMsfsRenderPasses.test.ts` around lines 88 - 96, Extend the test for canWriteReceiverGBufferFromProjectedGeometry to cover rejection when transparent, alphaHash, alphaTest, or vertexColors have non-default boolean or numeric values, while retaining the existing map and normalNode checks. Update the normalNode assignment to use the established as never cast style, matching createAircraftGBuffer.test.ts.
75-85: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case where
originalMaterialStatealready holds an entry.The test passes an empty
Map, sopreserveMaterialRenderStatesaves the current values and every??fallback inconfigureBlendGBufferMaterialsForDecalPassresolves to those same values. The branch that matters in production is the other one: the forward render path callsrestoreMaterialRenderStateand thenconfigureBlendGBufferMaterialsForDecalPasswith an already-populated map, and Line 936 restoresvisiblefrom the saved state rather than the current value.💚 Proposed additional test
+test('restores the saved visibility when the material state is already preserved', () => { + const material = new MeshBasicMaterial() + const state = new Map([[material as never, { + visible: true, + depthTest: false, + depthWrite: true, + polygonOffset: true + }]]) + material.visible = false + configureBlendGBufferMaterialsForDecalPass([material], state as never) + + expect(material.visible).toBe(true) + expect(material.depthTest).toBe(true) + expect(material.depthWrite).toBe(false) + expect(material.polygonOffset).toBe(true) +})🤖 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/rendering/createMsfsRenderPasses.test.ts` around lines 75 - 85, Add a test case alongside the existing decal occlusion test that passes a pre-populated originalMaterialState Map to configureBlendGBufferMaterialsForDecalPass, using saved material values that differ from the current material state. Assert that the configuration restores visible and other render properties from the saved entry, covering the restoreMaterialRenderState forward-render path rather than the empty-map fallback.src/rendering/createAircraftGBuffer.test.ts (1)
29-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the byte total from the attachment types instead of restating the literal.
expect(AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE).toBe(24)only repeats the constant definition. If an attachment changes fromUnsignedByteTypetoHalfFloatType, the layout doubles for that attachment but this test still passes. Compute the sum fromrenderTarget.texturesso the constant and the real layout stay linked.♻️ Proposed test change
- expect(AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE).toBe(24) + const bytesPerChannel = new Map([[UnsignedByteType, 1], [HalfFloatType, 2]]) + const measuredBytesPerSample = gBuffer.renderTarget.textures.reduce( + (total, texture) => total + (bytesPerChannel.get(texture.type) ?? 0) * 4, + 0 + ) + expect(measuredBytesPerSample).toBe(AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE) + expect(AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE).toBe(24)🤖 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/rendering/createAircraftGBuffer.test.ts` around lines 29 - 42, Update the byte-size assertion in the createAircraftGBuffer test to calculate the expected total from gBuffer.renderTarget.textures and each attachment’s type/bytes-per-channel metadata, rather than asserting the literal 24. Keep the existing texture order and type assertions unchanged so the AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE constant is validated against the actual render-target layout.src/main.ts (1)
4369-4381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGroup the optional boolean parameters into an options object.
loadAircraftModelDefinitionGltfhas one call site. The positional ninth and tenth parameters have defaults and do not identify their purpose at the call site. Use named options forprepareGltfInWorkerandbuildGBufferWriters.🤖 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/main.ts` around lines 4369 - 4381, Update loadAircraftModelDefinitionGltf to accept an options object for the optional boolean parameters, and change its call site to pass named prepareGltfInWorker and buildGBufferWriters values instead of positional ninth and tenth arguments. Preserve the existing defaults and behavior, including the exterior-kind mapping for buildGBufferWriters.src/rendering/createAircraftGBuffer.ts (1)
44-51: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueReuse the MRT probe node
three.js 0.183.0 provides
setBlendModeandgetBlendMode, so the deferred-path guard is valid. Create onemrt({})node and reuse it for both checks.🤖 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/rendering/createAircraftGBuffer.ts` around lines 44 - 51, Update supportsAircraftGBuffer to create a single mrt({}) probe node and reuse it for both setBlendMode and getBlendMode checks, while preserving the existing WebGPU backend validation and capability guard.
🤖 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 `@NOTES.md`:
- Line 27: The NOTES.md entry is outdated because it describes the removed
depth-mask/framebuffer depth-copy mechanism and misstates WebGPU support. Update
the 2026-07-27 entry to reflect that the deferred G-buffer path replaced the
blend-gbuffer receiver-depth mask approach, and accurately describe the current
createAircraftGBuffer behavior without claiming createMsfsRenderPasses rejects
WebGPU.
In `@src/msfs/gltf/normalizeMsfsMaterials.test.ts`:
- Around line 281-299: Correct the first receiver geometry in the test projects
decals only onto receiver faces matching their authored normal by adding the
missing third coordinate to its vertex data, making it a valid nine-component
triangle at z = 0 that faces away from the decal normal. Keep the sibling
receiver, decal geometry, and assertion unchanged.
In `@src/msfs/gltf/normalizeMsfsMaterials.ts`:
- Around line 1666-1712: Update rebuildConformedDecalGeometry in
src/msfs/gltf/normalizeMsfsMaterials.ts (lines 1666-1712) to select skinIndex
values from the dominant source vertex rather than passing them through
interpolateProjectionAttribute, while preserving Uint16Array when rewriting
skinIndex. Add a test case in src/msfs/gltf/normalizeMsfsMaterials.test.ts
(lines 597-627) covering a skinned decal over an unskinned receiver and
asserting integer skin indices copied from the source decal.
- Around line 3142-3144: Guard the source values before copying or constructing
nodes: only call writerMaterial.color.copy when sourceMaterial.color exists, and
only assign writerMaterial.emissive and invoke uniform for source.emissive when
it is defined. Update the emissive-node construction around
uniform(source.emissive) to skip absent emissive channels, preventing undefined
values from reaching node-type inference.
- Around line 2678-2685: Update the normal-scale handling around
applyMsfsBlendGBufferNodeMaterial and copyMsfsGBufferWriterInputs so the source
material’s normalScale remains unchanged for forward fallback decals. Clone the
aliased normalScale for the G-buffer writer before applying the full-strength
normal blend scale, while preserving the existing normalScale values and
blend-factor behavior elsewhere.
In `@src/rendering/createMsfsRenderPasses.ts`:
- Around line 99-102: The deferred capability gate in hasBlendGBufferMaterials
must exclude objects where isInteriorModelObject returns true, matching refresh
and the exterior-only G-buffer writer contract. In
src/rendering/createMsfsRenderPasses.ts:99-102, apply that filter; in
src/rendering/createMsfsRenderPasses.ts:145, defer createAircraftGBuffer until
refresh confirms an exterior decal and receiver, and add
MsfsRenderPasses.dispose to call gBuffer.dispose(). Make no change at
src/main.ts:4379-4379, which only documents the contract.
- Around line 408-414: Update the deferred base-pass setup around
setMeshMaterials and hideMaterials to also hide the meshes tracked in
forwardSourceMaterials, preventing forward-fallback decals from rendering with
their real materials before the forward decal pass. Keep the existing
restoreMeshMaterials(forwardSourceMaterials) call before that pass so those
materials are restored for the intended single render.
- Around line 582-591: Update syncResolveBatch to return whether batch
synchronization succeeded; preserve the existing matrix/visibility updates on
success and return false when the source or sharing validation fails. At the
call site that processes selected batches, detect a failed syncResolveBatch
result and apply resolve materials to that batch’s individual members so they
use the per-mesh resolve path instead of being omitted.
---
Nitpick comments:
In `@src/main.ts`:
- Around line 4369-4381: Update loadAircraftModelDefinitionGltf to accept an
options object for the optional boolean parameters, and change its call site to
pass named prepareGltfInWorker and buildGBufferWriters values instead of
positional ninth and tenth arguments. Preserve the existing defaults and
behavior, including the exterior-kind mapping for buildGBufferWriters.
In `@src/msfs/gltf/normalizeMsfsMaterials.test.ts`:
- Around line 140-147: Update the split-node handling around
evaluateCoverageNode so multi-component or otherwise unsupported node.components
values are rejected explicitly before indexing; validate that the swizzle
resolves to a single valid xyzw component, and throw a clear error instead of
returning undefined and allowing NaN downstream.
- Around line 597-627: The test around normalizeMsfsMaterials should also cover
an unskinned receiver with a skinned decal. Add a case where the receiver lacks
skinIndex and skinWeight while the decal has those attributes, then assert the
decal does not retain or generically interpolate its skin influences after
rebuildConformedDecalGeometry. Preserve the existing projected normal and
tangent assertions.
- Around line 189-193: Update the assertions in the affected
normalizeMsfsMaterials tests to assert values directly rather than comparing
boolean expressions with toBe(true). Replace count equality checks with direct
equality assertions and tolerance checks with toBeCloseTo or toBeLessThan,
preserving the existing expected values and thresholds so failures report the
actual operands.
- Around line 404-432: The existing G-buffer writer test lacks coverage for
independent ORM blend factors. Extend the test in the `builds G-buffer writers
only for node-compatible materials` case, or add a focused case, using a decal
material whose `ASOBO_material_blend_gbuffer` sets different roughness and
metallic factors (for example 0.5 and 1), and assert that
`getMsfsGBufferWriter(...)` returns null.
In `@src/msfs/gltf/normalizeMsfsMaterials.ts`:
- Around line 1093-1105: Clone the normal retrieved from projectionNormals
before any mutation in the triangle projection flow. Update the projectionNormal
initialization near sourceNormal so both mapped and fallback normals are copied,
preserving the existing negation and cross-product behavior without mutating
stored Vector3 instances.
In `@src/rendering/createAircraftGBuffer.test.ts`:
- Around line 29-42: Update the byte-size assertion in the createAircraftGBuffer
test to calculate the expected total from gBuffer.renderTarget.textures and each
attachment’s type/bytes-per-channel metadata, rather than asserting the literal
24. Keep the existing texture order and type assertions unchanged so the
AIRCRAFT_GBUFFER_BYTES_PER_SAMPLE constant is validated against the actual
render-target layout.
In `@src/rendering/createAircraftGBuffer.ts`:
- Around line 44-51: Update supportsAircraftGBuffer to create a single mrt({})
probe node and reuse it for both setBlendMode and getBlendMode checks, while
preserving the existing WebGPU backend validation and capability guard.
In `@src/rendering/createMsfsRenderPasses.test.ts`:
- Around line 88-96: Extend the test for
canWriteReceiverGBufferFromProjectedGeometry to cover rejection when
transparent, alphaHash, alphaTest, or vertexColors have non-default boolean or
numeric values, while retaining the existing map and normalNode checks. Update
the normalNode assignment to use the established as never cast style, matching
createAircraftGBuffer.test.ts.
- Around line 75-85: Add a test case alongside the existing decal occlusion test
that passes a pre-populated originalMaterialState Map to
configureBlendGBufferMaterialsForDecalPass, using saved material values that
differ from the current material state. Assert that the configuration restores
visible and other render properties from the saved entry, covering the
restoreMaterialRenderState forward-render path rather than the empty-map
fallback.
In `@src/rendering/createMsfsRenderPasses.ts`:
- Around line 633-652: Update the material eligibility predicate to reject
materials with a non-null lightMap, alongside aoMap and the other UV-dependent
map checks. Keep the existing acceptance behavior unchanged for materials
without lightMap.
- Around line 449-453: Resolve the redundant
configureDeferredDecalWriterDepthTest calls around the decal G-buffer pass and
its finally block. Since the helper only assigns depthTest = true, either add
the missing disable branch if the pass requires it, or remove the duplicate call
and rename the helper to reflect the invariant, preferably setting writer
material depth testing once during creation.
- Around line 397-403: The deferred render path should avoid per-frame scratch
allocations and redundant material lookups. In
src/rendering/createMsfsRenderPasses.ts:397-403, hoist the Color scratch object
to module scope beside drawingBufferSize, move originalMaterialState into the
render closure, and clear it at the start of render. In
src/rendering/createMsfsRenderPasses.ts:734-743, update the material-state
helper to iterate sourceMaterials entries once, reuse each entry’s material
value, and handle array materials without allocating a new array on every call.
- Around line 60-71: Update createMsfsRenderPasses to use
selectMsfsDecalRenderPath for its decal render-path decision, removing the
duplicate inline logic and keeping production behavior aligned with the tested
helper; alternatively remove the unused helper and update its tests if the
factory’s inline decision is the intended source of truth.
- Around line 539-563: Update syncResolveBatch to call updateMatrix() once for
each candidate mesh before invoking canShareResolveBatch, then remove the
transform updates from canShareResolveBatch. Keep canShareResolveBatch
side-effect-free while retaining its existing matrix and layer comparisons.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fa769bf-1a71-4cc3-a19c-a96916aa7f4e
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
NOTES.mdpackage.jsonsrc/main.tssrc/msfs/gltf/normalizeMsfsMaterials.test.tssrc/msfs/gltf/normalizeMsfsMaterials.tssrc/rendering/createAircraftGBuffer.test.tssrc/rendering/createAircraftGBuffer.tssrc/rendering/createMsfsRenderPasses.test.tssrc/rendering/createMsfsRenderPasses.ts
|
|
||
| ## Material And Geometry Findings | ||
|
|
||
| - 2026-07-27: Correct the earlier WebGPU depth-copy assumption: WebGPU is not fundamentally missing framebuffer/depth copying, and the installed Three.js r182 `WebGPURenderer` / `WebGPUBackend` implements `copyFramebufferToTexture`. The current blend-gbuffer receiver-depth mask is unavailable under WebGPU because `createMsfsRenderPasses.ts` explicitly rejects a WebGPU backend and also depends on the WebGL-only `getDrawingBufferSize` shape. A generic fix should validate destination depth format, physical dimensions, and multisampled-depth compatibility under WebGPU; if the canvas depth attachment cannot be copied into the sampled single-sample depth texture, use an explicit sampleable base-pass depth attachment rather than an aircraft-specific offset. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This note contradicts the code that lands in the same PR.
The entry states that createMsfsRenderPasses.ts "explicitly rejects a WebGPU backend and also depends on the WebGL-only getDrawingBufferSize shape". The new supportsAircraftGBuffer in src/rendering/createAircraftGBuffer.ts does the opposite: Line 47 requires isWebGPUBackend === true, and Line 81 calls renderer.getDrawingBufferSize on a WebGPURenderer.
The entry also describes the "blend-gbuffer receiver-depth mask" as the current mechanism. This PR removes the depth-mask and framebuffer depth-copy path.
Add the outcome to the entry so a future reader does not act on the superseded description. State that the deferred G-buffer path replaced the depth-mask approach.
🤖 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 `@NOTES.md` at line 27, The NOTES.md entry is outdated because it describes the
removed depth-mask/framebuffer depth-copy mechanism and misstates WebGPU
support. Update the 2026-07-27 entry to reflect that the deferred G-buffer path
replaced the blend-gbuffer receiver-depth mask approach, and accurately describe
the current createAircraftGBuffer behavior without claiming
createMsfsRenderPasses rejects WebGPU.
| test('projects decals only onto receiver faces matching their authored normal', async () => { | ||
| const root = new Group() | ||
| const parent = new Group() | ||
| parent.add(new Mesh(triangleGeometry([0, 0, 0, 0.2, 0, 0, 0.2, 0]), new MeshBasicMaterial())) | ||
| parent.add( | ||
| new Mesh( | ||
| triangleGeometry([0, 0, -0.1, 0, 0.2, -0.1, 0.2, 0, -0.1]), | ||
| new MeshBasicMaterial() | ||
| ) | ||
| ) | ||
| const decalGeometry = triangleGeometry([0, 0, 0.01, 0, 0.2, 0.01, 0.2, 0, 0.01]) | ||
| const decal = new Mesh(decalGeometry, blendGBufferMaterial()) | ||
| parent.add(decal) | ||
| root.add(parent) | ||
|
|
||
| await normalizeMsfsMaterials({ scene: root } as GLTF) | ||
|
|
||
| expect(Math.abs(decal.geometry.getAttribute('position').getZ(0) + 0.1) < 1e-6).toBe(true) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The first receiver geometry has 8 position components, so the test does not verify normal-based face selection.
Line 284 passes [0, 0, 0, 0.2, 0, 0, 0.2, 0]. That is 8 numbers with itemSize 3, which yields a fractional vertex count and a malformed attribute. The sibling receiver at Line 287 and the decal at Line 291 both use 9 numbers.
Because the z = 0 receiver is malformed, buildDecalProjectionTriangles cannot produce a usable triangle for it. The assertion at Line 298 then passes only because the z = -0.1 receiver is the sole candidate. The test does not exercise the authored-normal orientation filter it names.
Add the missing component so the z = 0 receiver is a valid triangle that faces away from the decal normal.
💚 Proposed fix
- parent.add(new Mesh(triangleGeometry([0, 0, 0, 0.2, 0, 0, 0.2, 0]), new MeshBasicMaterial()))
+ parent.add(
+ new Mesh(
+ triangleGeometry([0, 0, 0, 0.2, 0, 0, 0, 0.2, 0]),
+ new MeshBasicMaterial()
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test('projects decals only onto receiver faces matching their authored normal', async () => { | |
| const root = new Group() | |
| const parent = new Group() | |
| parent.add(new Mesh(triangleGeometry([0, 0, 0, 0.2, 0, 0, 0.2, 0]), new MeshBasicMaterial())) | |
| parent.add( | |
| new Mesh( | |
| triangleGeometry([0, 0, -0.1, 0, 0.2, -0.1, 0.2, 0, -0.1]), | |
| new MeshBasicMaterial() | |
| ) | |
| ) | |
| const decalGeometry = triangleGeometry([0, 0, 0.01, 0, 0.2, 0.01, 0.2, 0, 0.01]) | |
| const decal = new Mesh(decalGeometry, blendGBufferMaterial()) | |
| parent.add(decal) | |
| root.add(parent) | |
| await normalizeMsfsMaterials({ scene: root } as GLTF) | |
| expect(Math.abs(decal.geometry.getAttribute('position').getZ(0) + 0.1) < 1e-6).toBe(true) | |
| }) | |
| test('projects decals only onto receiver faces matching their authored normal', async () => { | |
| const root = new Group() | |
| const parent = new Group() | |
| parent.add( | |
| new Mesh( | |
| triangleGeometry([0, 0, 0, 0.2, 0, 0, 0, 0.2, 0]), | |
| new MeshBasicMaterial() | |
| ) | |
| ) | |
| parent.add( | |
| new Mesh( | |
| triangleGeometry([0, 0, -0.1, 0, 0.2, -0.1, 0.2, 0, -0.1]), | |
| new MeshBasicMaterial() | |
| ) | |
| ) | |
| const decalGeometry = triangleGeometry([0, 0, 0.01, 0, 0.2, 0.01, 0.2, 0, 0.01]) | |
| const decal = new Mesh(decalGeometry, blendGBufferMaterial()) | |
| parent.add(decal) | |
| root.add(parent) | |
| await normalizeMsfsMaterials({ scene: root } as GLTF) | |
| expect(Math.abs(decal.geometry.getAttribute('position').getZ(0) + 0.1) < 1e-6).toBe(true) | |
| }) |
🤖 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/msfs/gltf/normalizeMsfsMaterials.test.ts` around lines 281 - 299, Correct
the first receiver geometry in the test projects decals only onto receiver faces
matching their authored normal by adding the missing third coordinate to its
vertex data, making it a valid nine-component triangle at z = 0 that faces away
from the decal normal. Keep the sibling receiver, decal geometry, and assertion
unchanged.
| for (let component = 0; component < attribute.itemSize; component += 1) { | ||
| values.push(interpolateProjectionAttribute( | ||
| attribute, | ||
| vertex.sourceIndices, | ||
| vertex.sourceBarycentric, | ||
| component | ||
| )) | ||
| } | ||
| } | ||
|
|
||
| const influences = interpolateProjectedSkinInfluences( | ||
| vertex.receiverTriangle, | ||
| vertex.receiverBarycentric | ||
| ) | ||
| hasProjectedSkin ||= influences.length > 0 | ||
| for (let component = 0; component < 4; component += 1) { | ||
| projectedSkinIndices.push(influences[component]?.joint ?? 0) | ||
| projectedSkinWeights.push(influences[component]?.weight ?? 0) | ||
| } | ||
| } | ||
|
|
||
| for (const [name, attribute] of sourceAttributes) { | ||
| if ( | ||
| name === 'msfsBlendGBufferDepthAllowance' || | ||
| (hasProjectedSkin && (name === 'skinIndex' || name === 'skinWeight')) | ||
| ) { | ||
| continue | ||
| } | ||
| const values = outputAttributes.get(name) | ||
| if (values != null) { | ||
| geometry.setAttribute( | ||
| name, | ||
| new BufferAttribute(new Float32Array(values), attribute.itemSize) | ||
| ) | ||
| } | ||
| } | ||
| geometry.deleteAttribute('msfsBlendGBufferDepthAllowance') | ||
| if (hasProjectedSkin) { | ||
| geometry.setAttribute( | ||
| 'skinIndex', | ||
| new BufferAttribute(new Uint16Array(projectedSkinIndices), 4) | ||
| ) | ||
| geometry.setAttribute( | ||
| 'skinWeight', | ||
| new BufferAttribute(new Float32Array(projectedSkinWeights), 4) | ||
| ) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Categorical skinIndex values pass through barycentric interpolation. rebuildConformedDecalGeometry treats every non-special attribute as interpolatable. Joint indices are identifiers, not scalars, so blending joint 2 and joint 7 yields 4.5 and binds the vertex to an unrelated bone. The path is reachable whenever interpolateProjectedSkinInfluences returns an empty list, that is, when the receiver carries no skin influences.
src/msfs/gltf/normalizeMsfsMaterials.ts#L1666-L1712: select the dominant source vertex forskinIndexinstead of interpolating it, and keep theUint16Arraytype when rewriting the attribute at Line 1698.src/msfs/gltf/normalizeMsfsMaterials.test.ts#L597-L627: add a case with a skinned decal over an unskinned receiver, and assert that the resultingskinIndexvalues are integers taken from the source decal.
📍 Affects 2 files
src/msfs/gltf/normalizeMsfsMaterials.ts#L1666-L1712(this comment)src/msfs/gltf/normalizeMsfsMaterials.test.ts#L597-L627
🤖 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/msfs/gltf/normalizeMsfsMaterials.ts` around lines 1666 - 1712, Update
rebuildConformedDecalGeometry in src/msfs/gltf/normalizeMsfsMaterials.ts (lines
1666-1712) to select skinIndex values from the dominant source vertex rather
than passing them through interpolateProjectionAttribute, while preserving
Uint16Array when rewriting skinIndex. Add a test case in
src/msfs/gltf/normalizeMsfsMaterials.test.ts (lines 597-627) covering a skinned
decal over an unskinned receiver and asserting integer skin indices copied from
the source decal.
| const normalBlendFactor = | ||
| isBlendGBufferMaterial && buildGBufferWriter | ||
| ? 1 | ||
| : blendFactors.normal | ||
| outputMaterial.normalScale.set( | ||
| outputMaterial.normalScale.x * blendFactors.normal, | ||
| -Math.abs(outputMaterial.normalScale.y) * blendFactors.normal | ||
| outputMaterial.normalScale.x * normalBlendFactor, | ||
| -Math.abs(outputMaterial.normalScale.y) * normalBlendFactor | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(normalizeMsfsMaterials|createMsfsRenderPasses)\.(ts|tsx|js|jsx)$|gltf'
printf '%s\n' '--- target outline ---'
ast-grep outline src/msfs/gltf/normalizeMsfsMaterials.ts | head -200
printf '%s\n' '--- target lines ---'
sed -n '2640,2710p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3000,3070p' src/msfs/gltf/normalizeMsfsMaterials.ts
printf '%s\n' '--- render-pass outline ---'
ast-grep outline src/rendering/createMsfsRenderPasses.ts | head -240
printf '%s\n' '--- relevant render-pass references ---'
rg -n -C 8 'forwardDecals|deferred|decal\.material|msfsGBufferWriter|normalScale|blendFactors' src/rendering/createMsfsRenderPasses.ts src/msfs/gltf/normalizeMsfsMaterials.tsRepository: jamesx0416/FlightSim
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(normalizeMsfsMaterials|createMsfsRenderPasses)\.(ts|tsx|js|jsx)$|gltf'
printf '%s\n' '--- target outline ---'
ast-grep outline src/msfs/gltf/normalizeMsfsMaterials.ts | head -200
printf '%s\n' '--- target lines ---'
sed -n '2640,2710p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3000,3070p' src/msfs/gltf/normalizeMsfsMaterials.ts
printf '%s\n' '--- render-pass outline ---'
ast-grep outline src/rendering/createMsfsRenderPasses.ts | head -240
printf '%s\n' '--- relevant render-pass references ---'
rg -n -C 8 'forwardDecals|deferred|decal\.material|msfsGBufferWriter|normalScale|blendFactors' \
src/rendering/createMsfsRenderPasses.ts \
src/msfs/gltf/normalizeMsfsMaterials.tsRepository: jamesx0416/FlightSim
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- normalization entry and call sites ---'
sed -n '340,465p' src/msfs/gltf/normalizeMsfsMaterials.ts
rg -n -C 10 'normalizeMsfsMaterial\(|buildGBufferWriter|createNodeMaterial' \
src/msfs/gltf/normalizeMsfsMaterials.ts \
src/msfs/gltf/createMsfsGltfLoader.ts
printf '%s\n' '--- node material and normal construction ---'
sed -n '2890,2965p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3135,3175p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3335,3465p' src/msfs/gltf/normalizeMsfsMaterials.ts
printf '%s\n' '--- forward pass material flow ---'
sed -n '220,325p' src/rendering/createMsfsRenderPasses.ts
sed -n '450,510p' src/rendering/createMsfsRenderPasses.ts
sed -n '719,805p' src/rendering/createMsfsRenderPasses.ts
sed -n '915,950p' src/rendering/createMsfsRenderPasses.ts
printf '%s\n' '--- focused tests and references ---'
rg -n -C 8 'normalScale|normalBlend|GBufferWriter|forward|blend.*normal|normal' \
src/msfs/gltf/normalizeMsfsMaterials.test.ts \
src/rendering --glob '*test*' || trueRepository: jamesx0416/FlightSim
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- exact blend-node implementation ---'
sed -n '3190,3230p' src/msfs/gltf/normalizeMsfsMaterials.ts
printf '%s\n' '--- exact normal/writer implementation ---'
sed -n '2990,3078p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3138,3158p' src/msfs/gltf/normalizeMsfsMaterials.ts
sed -n '3350,3442p' src/msfs/gltf/normalizeMsfsMaterials.ts
printf '%s\n' '--- loader options and node factory ---'
rg -n -C 12 'normalizeMsfsMaterials|createNodeMaterial|buildGBufferWriters' \
src --glob '*.ts' --glob '*.tsx' | head -240
printf '%s\n' '--- focused test fixtures and writer assertions ---'
sed -n '1,90p' src/msfs/gltf/normalizeMsfsMaterials.test.ts
rg -n -C 12 'createMsfsDeferredLightingMaterial|getMsfsGBufferWriter|buildGBuffer|nodeMaterial|normalScale|blendFactors' \
src/msfs/gltf/normalizeMsfsMaterials.test.ts
printf '%s\n' '--- diff summary and relevant changed lines ---'
git diff --stat
git diff -- src/msfs/gltf/normalizeMsfsMaterials.ts src/rendering/createMsfsRenderPasses.ts | \
rg -n -C 5 'normalScale|buildGBuffer|forwardSource|msfsGBufferWriter|aircraftG1|blendFactors' | head -260 || true
printf '%s\n' '--- read-only data-flow verifier ---'
python3 - <<'PY'
from pathlib import Path
normalize = Path("src/msfs/gltf/normalizeMsfsMaterials.ts").read_text()
passes = Path("src/rendering/createMsfsRenderPasses.ts").read_text()
checks = {
"normal_scale_mutates_input_before_replacement": (
"let outputMaterial = material" in normalize
and "outputMaterial.normalScale.set(" in normalize
and normalize.index("outputMaterial.normalScale.set(")
< normalize.index("const nodeMaterial = ensureNodeMaterial")
),
"writer_uses_source_normal_scale": (
"createMsfsGBufferNormalNode(source)" in normalize
and "const normalNode = vec3(resolveMsfsMaterialNode(" in normalize
),
"writer_applies_normal_coverage_factor": (
"aircraftG1: vec4(normalNode, isDecal ? coverage(blendFactors?.normal ?? 1)" in normalize
),
"forward_path_stores_original_material": (
"forwardSourceMaterials.set(decal, decal.material)" in passes
and "restoreMeshMaterials(forwardSourceMaterials)" in passes
),
"forward_path_configures_same_material": (
"configureBlendGBufferMaterialsForDecalPass(" in passes
and "forwardColorMaterials.add(material)" in passes
),
"forward_blend_node_does_not_set_normal_factor": (
"nodeMaterial.colorNode = vec4(" in normalize
and "normalNode" not in normalize[
normalize.index("function applyMsfsBlendGBufferNodeMaterial"):
normalize.index("function getMsfsBlendGBufferForwardOpacityFactor")
]
),
}
for name, result in checks.items():
print(f"{name}: {result}")
PYRepository: jamesx0416/FlightSim
Length of output: 33785
🏁 Script executed:
#!/bin/bash
set -eu
sed -n '3098,3148p' src/msfs/gltf/normalizeMsfsMaterials.ts
python3 - <<'PY'
from pathlib import Path
text = Path("src/msfs/gltf/normalizeMsfsMaterials.ts").read_text()
start = text.index("function copyMsfsGBufferWriterInputs")
end = text.index("function createMsfsGBufferNormalNode")
print(text[start:end])
PYRepository: jamesx0416/FlightSim
Length of output: 2263
Preserve the blend factor for forward fallback normals
When a deferred relationship is rejected, the forward decal reuses this material. Since applyMsfsBlendGBufferNodeMaterial does not apply blendFactors.normal, the decal renders with full-strength normals. Keep the source material unchanged and apply the full-strength scale only to a cloned normalScale on the G-buffer writer; copyMsfsGBufferWriterInputs currently aliases the source vector.
🤖 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/msfs/gltf/normalizeMsfsMaterials.ts` around lines 2678 - 2685, Update the
normal-scale handling around applyMsfsBlendGBufferNodeMaterial and
copyMsfsGBufferWriterInputs so the source material’s normalScale remains
unchanged for forward fallback decals. Clone the aliased normalScale for the
G-buffer writer before applying the full-strength normal blend scale, while
preserving the existing normalScale values and blend-factor behavior elsewhere.
| writerMaterial.color?.copy?.(sourceMaterial.color) | ||
| writerMaterial.emissive = sourceMaterial.emissive | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard emissive before you build the emissive node.
Line 3143 copies sourceMaterial.emissive by direct assignment. A node material converted from a source without an emissive channel, for example a MeshBasicNodeMaterial, has no emissive property. writerMaterial.emissive then becomes undefined.
Line 3186 calls uniform(source.emissive) with that value. uniform cannot infer a node type from undefined.
Line 3142 has the same exposure in reverse: writerMaterial.color.copy(sourceMaterial.color) runs when sourceMaterial.color is undefined, because the optional chain only guards the target side.
🛡️ Proposed fix
- writerMaterial.color?.copy?.(sourceMaterial.color)
- writerMaterial.emissive = sourceMaterial.emissive
+ if (sourceMaterial.color != null) {
+ writerMaterial.color?.copy?.(sourceMaterial.color)
+ }
+ if (sourceMaterial.emissive != null) {
+ writerMaterial.emissive = sourceMaterial.emissive
+ } function createMsfsGBufferEmissiveNode(material: MsfsMaterial) {
const source = material as any
+ if (source.emissive == null) {
+ return vec3(0)
+ }
let emissiveNode = vec3(uniform(source.emissive)).mul(source.emissiveIntensity ?? 1)Also applies to: 3184-3191
🤖 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/msfs/gltf/normalizeMsfsMaterials.ts` around lines 3142 - 3144, Guard the
source values before copying or constructing nodes: only call
writerMaterial.color.copy when sourceMaterial.color exists, and only assign
writerMaterial.emissive and invoke uniform for source.emissive when it is
defined. Update the emissive-node construction around uniform(source.emissive)
to skip absent emissive channels, preventing undefined values from reaching
node-type inference.
| if (!hasBlendGBufferMaterials(root) || !supportsAircraftGBuffer(renderer)) { | ||
| return fallback | ||
| } | ||
| return createDeferredMsfsRenderPasses(renderer, scene, camera, root, fallback) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
The deferred capability gate ignores the interior filter that the rest of the path applies. hasBlendGBufferMaterials(root) traverses every mesh under the aircraft root, including interior meshes. refresh then skips interior meshes at Line 217 through isInteriorModelObject, and src/main.ts builds G-buffer writers only for exterior components. For a model whose blend-G-buffer materials are all in the interior, the gate passes, createAircraftGBuffer allocates four full-screen attachments plus a depth texture, refresh collects no decals, canRenderDeferred stays false, and every frame falls back to the forward renderer. The allocation is never used and AircraftGBuffer.dispose is never called.
src/rendering/createMsfsRenderPasses.ts#L99-L102: apply the same interior exclusion in the gate, for example by skipping objects for whichisInteriorModelObjectreturnstrueinsidehasBlendGBufferMaterials.src/rendering/createMsfsRenderPasses.ts#L145-L145: defercreateAircraftGBufferuntil the firstrefreshproves at least one exterior decal and receiver exist, and add adisposemethod toMsfsRenderPassesthat callsgBuffer.dispose().src/main.ts#L4379-L4379: no change needed here; this line documents the exterior-only writer contract that the gate should match.
📍 Affects 2 files
src/rendering/createMsfsRenderPasses.ts#L99-L102(this comment)src/rendering/createMsfsRenderPasses.ts#L145-L145src/main.ts#L4379-L4379
🤖 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/rendering/createMsfsRenderPasses.ts` around lines 99 - 102, The deferred
capability gate in hasBlendGBufferMaterials must exclude objects where
isInteriorModelObject returns true, matching refresh and the exterior-only
G-buffer writer contract. In src/rendering/createMsfsRenderPasses.ts:99-102,
apply that filter; in src/rendering/createMsfsRenderPasses.ts:145, defer
createAircraftGBuffer until refresh confirms an exterior decal and receiver, and
add MsfsRenderPasses.dispose to call gBuffer.dispose(). Make no change at
src/main.ts:4379-4379, which only documents the contract.
| renderer.setRenderTarget(originalTarget) | ||
| renderer.autoClear = true | ||
| camera.layers.mask = originalCameraLayerMask | ||
| setMeshMaterials(sourceMaterials, hiddenMaterial) | ||
| setMeshMaterialsFor(receivers, sourceMaterials) | ||
| hideMaterials(transparentSceneMaterials, originalMaterialState) | ||
| renderer.render(scene, camera) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Forward-fallback decals render twice per frame.
Line 411 hides every mesh tracked in sourceMaterials, and Line 412 restores the receivers. The meshes tracked in forwardSourceMaterials are not hidden here, so they render in this base pass with their real transparent color materials.
Lines 475-485 then restore those same meshes and render them again in the forward decal pass. Each forward-fallback decal therefore composites twice into the framebuffer. For alpha-blended or additive decal materials the visible result is a doubled contribution.
The forward-only path avoids this: Line 826 adds the color blend materials to hiddenBlendMaterials, so the forward base pass at Line 872 skips them. The deferred path needs the same exclusion.
🐛 Proposed fix
setMeshMaterials(sourceMaterials, hiddenMaterial)
+ setMeshMaterials(forwardSourceMaterials, hiddenMaterial)
setMeshMaterialsFor(receivers, sourceMaterials)
hideMaterials(transparentSceneMaterials, originalMaterialState)
renderer.render(scene, camera)Note that Line 476 already calls restoreMeshMaterials(forwardSourceMaterials) before the forward decal pass, so the added hide is reverted at the right point.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| renderer.setRenderTarget(originalTarget) | |
| renderer.autoClear = true | |
| camera.layers.mask = originalCameraLayerMask | |
| setMeshMaterials(sourceMaterials, hiddenMaterial) | |
| setMeshMaterialsFor(receivers, sourceMaterials) | |
| hideMaterials(transparentSceneMaterials, originalMaterialState) | |
| renderer.render(scene, camera) | |
| renderer.setRenderTarget(originalTarget) | |
| renderer.autoClear = true | |
| camera.layers.mask = originalCameraLayerMask | |
| setMeshMaterials(sourceMaterials, hiddenMaterial) | |
| setMeshMaterials(forwardSourceMaterials, hiddenMaterial) | |
| setMeshMaterialsFor(receivers, sourceMaterials) | |
| hideMaterials(transparentSceneMaterials, originalMaterialState) | |
| renderer.render(scene, camera) |
🤖 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/rendering/createMsfsRenderPasses.ts` around lines 408 - 414, Update the
deferred base-pass setup around setMeshMaterials and hideMaterials to also hide
the meshes tracked in forwardSourceMaterials, preventing forward-fallback decals
from rendering with their real materials before the forward decal pass. Keep the
existing restoreMeshMaterials(forwardSourceMaterials) call before that pass so
those materials are restored for the intended single render.
| function syncResolveBatch(batch: ResolveBatch): void { | ||
| const source = batch.members[0] | ||
| if (source == null || !batch.members.every(member => canShareResolveBatch(source, member))) { | ||
| return | ||
| } | ||
| source.updateMatrix() | ||
| batch.mesh.matrix.copy(source.matrix) | ||
| batch.mesh.matrixWorldNeedsUpdate = true | ||
| batch.mesh.visible = true | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
A failed batch sync silently drops the decals from the resolve pass.
At Line 462 setMeshMaterialsFor(selectedBatches.unbatched, resolveMaterials) assigns resolve materials only to the unbatched decals. Every member of a selected batch is excluded from unbatched.
Line 464 then calls syncResolveBatch. If the guard at Line 584 fails, the function returns early and batch.mesh.visible stays false. The batch members have no resolve material assigned and the batch mesh does not draw. Those decals wrote into the G-buffer but never resolve, so they disappear for that frame.
Return a success flag and fall back to per-mesh resolve for the members.
🐛 Proposed fix
-function syncResolveBatch(batch: ResolveBatch): void {
+function syncResolveBatch(batch: ResolveBatch): boolean {
const source = batch.members[0]
if (source == null || !batch.members.every(member => canShareResolveBatch(source, member))) {
- return
+ return false
}
source.updateMatrix()
batch.mesh.matrix.copy(source.matrix)
batch.mesh.matrixWorldNeedsUpdate = true
batch.mesh.visible = true
+ return true
}Then handle the failure at the call site:
setMeshMaterialsFor(selectedBatches.unbatched, resolveMaterials)
for (const batchIndex of selectedBatches.batchIndices) {
- syncResolveBatch(resolveBatches[batchIndex])
+ const batch = resolveBatches[batchIndex]
+ if (!syncResolveBatch(batch)) {
+ setMeshMaterialsFor(batch.members, resolveMaterials)
+ }
}🤖 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/rendering/createMsfsRenderPasses.ts` around lines 582 - 591, Update
syncResolveBatch to return whether batch synchronization succeeded; preserve the
existing matrix/visibility updates on success and return false when the source
or sharing validation fails. At the call site that processes selected batches,
detect a failed syncResolveBatch result and apply resolve materials to that
batch’s individual members so they use the per-mesh resolve path instead of
being omitted.
1 participant
Summary
Validation
bun testbun run typecheckSummary by CodeRabbit
New Features
Bug Fixes
Performance