Rendering pipeline: EXR textures, shadows, frame graph, IBL#31
Merged
Conversation
- Scene hierarchy: stop calling ScrollToItem every frame; only auto-scroll once after picking changes selection - Zoom curve: reduce min distance from 0.1m to 0.01m, increase far plane, allowing closer zoom in all scale scenes - Object creation: intersect click ray with y=0 ground plane instead of using arbitrary ndc_z=0.95 depth - Selection in scene cameras: move picking before the early return that blocks input for non-free cameras
…rces Split IRenderPass into IPass (lifecycle base) and ITopLevelPass (generic add_to_frame_graph virtual). Sub-passes inherit IPass directly with typed Inputs/Outputs contracts — parent renderers wire them explicitly via structured bindings, eliminating magic string-based resource coupling. Extend FrameGraph with buffer and bind group resources alongside textures: - CRTP CachedResource<T> base with version tracking and ResourceRef<T> - BufferHandle/BufferRef with find_or_create_buffer() and import_buffer() - BindGroupHandle/BindGroupRef with version-based auto-invalidation - Array texture support (TextureDesc::array_layers, per-layer views) - Auto-namespaced resources by pass name with WebGPU debug labels - IPass helpers: create_texture/create_buffer/import_buffer/create_bind_group - Layer-targeted attachments always clear (each layer is independent) Migrate all passes to frame-graph-managed resources: - Eliminate manual cached_* dirty-tracking fields - Eliminate per-frame bind group creation inside execute lambdas - Delete ShadowPassData cross-pass side channel - Shadow texture array managed by frame graph with layer views
Shadow uniform split: Separate model matrices from light VPs in the shadow pass. Model matrices uploaded once per frame, shared across all shadow layers. Reduces upload from L*N*256 to (N+L)*256 bytes. Auto-exposure: Luminance compute pass derives EV offset from average scene luminance. Uses depth buffer to mask sky pixels — stable at any camera angle. Temporal smoothing with configurable adaptation speed. Manual EV slider becomes an additive bias. Handles USD physical light values (DistantLight default 50,000 lux). Distant light gizmo: Circle + direction arrow wireframe rendered by EditorPass. Pickable via line-list pipeline. Placed at the light's USD transform position.
IRenderer is the sole abstraction boundary: NVI add_to_frame_graph calls
virtual do_add_to_frame_graph (returns HdrOutputs — compile-time contract),
then applies built-in tone mapping + auto-exposure. IPass has no frame
graph virtual. ToneMappingPass moved to core, owned by IRenderer.
Remove all magic string coupling between editor and renderers:
- No find_or_create("scene_color"/"scene_depth"/"tone_mapped_color") in editor
- Editor reads outputs exclusively via IRenderer::Outputs return value
- Renderers use create_texture (auto-namespaced) for internal resources
- ToneMappingPass receives depth via typed Inputs, not fg.find()
- Fix path tracer crash: guard depth dereference in luminance pass
- Optional depth (std::optional<TextureHandle>) in Outputs
Editor passes (Grid, Editor, Lobe) called explicitly through concrete
types with typed render() methods. for_each_pass used only for lifecycle.
Delete void* pointer comparison hack.
Every pass now has a logger accessible via logger(). Shared from the ShaderLoader's logger (same spdlog instance used by the application). No constructor signature changes — IPass reads it from ShaderLoader.
- Add OpenEXR as Conan dependency for full EXR compression support (DWAA, DWAB, PXR24, B44, ZIP, PIZ, RLE) - Unified load_image_float(): EXR via OpenEXR, HDR via stbi_loadf, LDR via stbi_load (no double-linearization from stbi_loadf gamma) - Custom MemoryIStream for reading EXR from USD asset buffers - Upgrade material texture array from RGBA8Unorm to RGBA16Float - Extract float_to_half() into shared halfFloat.h - Cache failed texture loads to avoid repeated decode attempts - Add treasure chest model with EXR textures (metal, roughness, normal)
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors pass API from IRenderPass→IPass, moves per-pass GPU resources into FrameGraph (textures/buffers/bind-groups, per-layer views), adds auto‑exposure/tone‑mapping with a luminance compute shader, introduces half‑float/EXR image handling, and updates many passes, editor code, tests, and CI/build configs. Changes
Sequence Diagram(s)sequenceDiagram
participant Renderer as Renderer
participant FrameGraph as FrameGraph
participant Pass as IPass
participant GPU as GPU
Renderer->>FrameGraph: add_to_frame_graph(pass)
FrameGraph->>Pass: invoke pass -> create_texture/create_buffer/create_bind_group
Pass-->>FrameGraph: return Outputs (TextureHandle/BufferHandle)
FrameGraph->>FrameGraph: compile() -> allocate_buffers()/allocate_bind_groups()
FrameGraph->>GPU: execute passes (resolve handles -> real GPU objects)
GPU-->>FrameGraph: complete
FrameGraph->>FrameGraph: evict_unused()
sequenceDiagram
participant Editor as EditorApplication
participant Renderer as Renderer
participant ToneMap as ToneMappingPass
participant Luminance as LuminanceCompute
participant FrameGraph as FrameGraph
participant GPU as GPU
Editor->>Renderer: add_to_frame_graph(fg, ctx)
Renderer->>FrameGraph: produce hdr_color, depth handles
ToneMap->>FrameGraph: create buffers + bind groups
ToneMap->>Luminance: schedule compute pass (dispatch)
Luminance->>FrameGraph: write ExposureResult
ToneMap->>FrameGraph: tone-map render (reads hdr + exposure_result -> ldr)
FrameGraph->>GPU: execute compute + render
GPU-->>FrameGraph: results ready
ToneMap-->>Editor: display_color_handle
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/rendering/renderWorld.cpp (1)
603-604:⚠️ Potential issue | 🟠 MajorLight proxy changes must invalidate the TLAS.
Line 631-L648 adds visible lights to
instances, but Line 603-L604 still rebuilds the TLAS only on object-transform and mesh changes. Moving/hiding a light, changing its proxy mesh, or updating its material now leavesdata.tlasandgpu_instancesstale until some unrelated object or mesh edit forces a rebuild. This needs a dedicated cached light-geometry invalidation signal in the rebuild condition.Also applies to: 631-648
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/src/rendering/renderWorld.cpp` around lines 603 - 604, The TLAS rebuild condition only checks object transform/mesh versions (need_rebuild uses any_blas_dirty, m_transform_version vs m_cached_transform_version, and m_mesh_version vs m_cached_geometry_version) but not light proxy changes; add a dedicated light-change signal (e.g., m_lights_version and m_cached_lights_version or any_lights_dirty) and include it in the need_rebuild expression so changes to light position/visibility, proxy mesh, or material (the code that appends visible lights to instances around the visible-lights loop) will trigger TLAS/gpu_instances rebuilds; ensure whatever code updates light proxies increments the new lights version so data.tlas and gpu_instances are invalidated correctly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 115-117: Split the conditional PTSTUDIO_GPU_BACKEND env assignment
into platform-specific steps to avoid emitting a spurious warning on non-Windows
runs: remove the inline ternary-style assignment PTSTUDIO_GPU_BACKEND: ${{
matrix.platform == 'windows-x64' && 'D3D12' || '' }} and instead create/modify
the two workflow steps named "Build project" and "Run tests" so each has a
Windows-only variant guarded by if: matrix.platform == 'windows-x64' that sets
PTSTUDIO_GPU_BACKEND: D3D12, and leave the non-Windows variants without that env
var; ensure the steps still use the same job matrix and runner so behavior is
unchanged for Windows while non-Windows runs no longer set an empty/unknown
PTSTUDIO_GPU_BACKEND.
In `@allowlist_extra.toml`:
- Around line 18-20: The allowlist contains an unrestricted entry named
"image_convert" that permits running "ffmpeg" but no code references it; remove
this unnecessary attack surface by deleting the [[allow]] block with name
"image_convert" and commands ["ffmpeg"] from allowlist_extra.toml, or if ffmpeg
is actually required in future, replace the open command entry with a scoped
allow that adds a restrictive "patterns" array limiting executable arguments and
file paths (e.g., explicit input/output file patterns) instead of permitting all
ffmpeg usage.
In `@assets/scenes/area_light_test.usda`:
- Around line 122-127: The prim for treasure_chest_2k contains inert transform
channels (xformOp:rotateX:unitsResolve, xformOp:rotateXYZ, xformOp:scale,
xformOp:translate) while only matrix4d xformOp:transform is listed in
xformOpOrder; remove the unused rotate/scale/translate attributes from the
treasure_chest_2k prim so the file only authoritatively contains
xformOp:transform (and ensure xformOpOrder remains ["xformOp:transform"]) to
avoid contradictory/ignored ops during edits and round-trips.
In `@assets/scenes/camera.usda`:
- Line 69: The packager still converts assets/scenes/camera/camera.usda
(configured in config.yaml) into the .usdz used in builds, so the fixes you made
in assets/scenes/camera.usda (e.g., updated prepend references to
../models/camera/Camera_01_2k.usdc) are not included; update either config.yaml
to point to the edited file (assets/scenes/camera/camera.usda) or move your
edits into the scene file that config.yaml actually converts
(assets/scenes/camera/camera.usda) so the corrected model/HDR paths are
packaged—search for the config entry in config.yaml and the prepend references
in camera.usda to apply the change consistently.
In `@assets/scenes/shadow_test.usda`:
- Around line 120-121: The xformOpOrder currently lists only
"xformOp:transform", which leaves existing xformOp:rotateXYZ entries on MainSun
and FillSun orphaned and inert; update the xformOpOrder to include the rotate
ops in the correct order (e.g., include "xformOp:rotateXYZ" entries alongside
"xformOp:transform") for both MainSun and FillSun so the rotate values are
applied (also check and fix the same xformOpOrder at the other occurrence noted
around lines 130-131).
In `@core/include/core/rendering/aabb.h`:
- Around line 16-19: empty() currently only checks the x-axis and can disagree
with is_valid(), allowing boxes invalid on y/z to appear non-empty; change
empty() to be consistent with is_valid() by either returning !is_valid() or by
checking all three axes (min.x > max.x || min.y > max.y || min.z > max.z).
Update the AABB::empty() implementation (and any related comments) so it
references the same validity criteria as is_valid(), and ensure from_min_max()
behavior remains unchanged.
In `@core/include/core/rendering/frameGraph.h`:
- Around line 61-73: The BindGroupEntry struct currently allows multiple
resource fields to be set despite the comment "Exactly one of these is set per
entry"; add an explicit enforcement and documentation: implement a validation
helper method (e.g., BindGroupEntry::Validate() or IsExclusive()) that checks
exactly one of the resource discriminants (buffer, texture, sampler,
external_view, external_buffer) is non-default and returns/asserts/fails if not,
or replace the raw fields with factory functions (e.g.,
BindGroupEntry::ForBuffer(...), ForTexture(...), ForExternalView(...)) that
construct entries with only the appropriate fields populated; update the struct
comment to list valid combinations and reference the fields buffer,
buffer_offset, buffer_size, texture, texture_layer, sampler, external_view,
external_buffer, external_buffer_size so callers use the validator/factories to
guarantee the invariant.
- Around line 165-167: BufferRef::size() currently returns std::size_t while it
reads BufferDesc::size (a uint64_t), risking truncation on 32-bit platforms;
change the BufferRef::size() return type to uint64_t (update the signature in
frameGraph.h for the size() method and any callers that expect std::size_t) so
it returns m_cached ? m_cached->desc.size : 0ULL and preserves the full 64-bit
value from BufferDesc::size.
In `@core/include/core/rendering/halfFloat.h`:
- Around line 22-24: The NaN-to-inf corruption happens in the exponent-all-ones
branch inside halfFloat conversion (the block that checks exponent == 0xFF - 127
+ 15); when mantissa >> 13 yields 0 we currently emit 0x7C00 (inf) losing NaN
payload — change the emission to preserve a nonzero NaN payload by OR-ing a
minimal payload bit into the half mantissa field (e.g., compute halfMantissa =
(mantissa >> 13) ? (mantissa >> 13) : 1 and then return sign | 0x7C00 |
halfMantissa) so low-payload NaNs remain NaNs rather than becoming infinity.
In `@core/include/core/rendering/shaderLoader.h`:
- Around line 14-16: Remove the unused forward declaration of class
LoggingManager inside namespace pts in shaderLoader.h; locate the forward
declaration "namespace pts { class LoggingManager; }" and delete that
declaration (it is not referenced in shaderLoader.h or its implementation),
leaving the rest of the namespace and includes untouched.
In `@core/include/core/rendering/toneMappingPass.h`:
- Around line 40-43: Mark the accessor ldr_output() as [[nodiscard]] to prevent
callers from accidentally discarding the produced TextureHandle; update the
declaration of ldr_output() (which returns TextureHandle and returns
m_ldr_output) by adding the [[nodiscard]] attribute before the function's return
type/signature while keeping it const and its semantics (valid after
add_to_frame_graph).
In `@core/src/rendering/camera.cpp`:
- Around line 42-45: m_near is currently being set equal to m_min_distance which
forces an excessively small projection near plane; change the initialization so
m_min_distance remains the small orbit floor (keep m_min_distance = 0.01f * s)
but assign m_near independently to a larger, projection-appropriate value (for
example 0.1f * s or another suitable minimum) so that the value passed into
glm::perspective() is not the tiny orbit floor and depth precision is preserved.
- Around line 63-65: The zoom method (OrbitCamera::zoom) currently multiplies
m_distance and only clamps it against m_near, which allows zooming past
m_max_distance and uses the wrong lower bound; change the clamp to enforce
m_distance remains within [m_min_distance, m_max_distance] after updating it so
pan() scaling and interactions with set_clip_planes()/set_distance() remain
stable; locate OrbitCamera::zoom and replace the existing post-update clamp to
use m_min_distance and m_max_distance.
In `@core/src/rendering/frameGraph.cpp`:
- Around line 707-708: The precondition in allocate_bind_groups is blocking use
of entry.texture_layer (PRECONDITION_MSG(entry.texture_layer == UINT32_MAX,
"allocate_bind_groups: texture_layer not yet supported")); create a tracked task
and add a TODO comment at that location referencing the issue number so future
work is discoverable, and update allocate_bind_groups, BindGroupEntry handling,
and any related classes to support texture_layer when implemented; use the
unique symbols entry.texture_layer and allocate_bind_groups in the task
description so implementers know exactly where to add support.
- Around line 614-618: The reuse check in frameGraph (m_buffer_cache lookup) is
too strict by requiring exact equality of desc.usage; update the condition in
the block that currently compares it->second->desc.usage == res.desc.usage
(inside the reuse branch that sets it->second->used_this_frame) to allow cached
buffers whose usage flags are a superset of the requested flags — i.e., replace
the equality test with a bitmask superset test that ensures (cached_usage &
required_usage) == required_usage so buffers with extra capabilities (merged by
find_or_create_buffer) are reused instead of recreated while still keeping the
existing size and used_this_frame logic.
- Around line 349-377: Replace the fragile C-string dispatch in
FrameGraph::make_pass_key by using a typed enum (e.g., ResourceType { Texture,
Buffer, BindGroup }) instead of passing char* type_prefix; change the function
signature or add an overload that accepts the enum, switch on that enum to
select counters.texture / counters.buffer / counters.bind_group (instead of
strcmp), and keep a string-label overload that maps known literals to the enum
with a checked lookup to fail fast on unknown values; also fix the reserve
sizing by computing the actual digit length for the counter (e.g., use
std::to_string(n).size() or reserve a safe upper bound like 11) when building
the suffix so large counters don’t under-allocate.
In `@core/src/rendering/renderPass.cpp`:
- Around line 215-219: When tone mapping is bypassed (m_tonemapping_enabled is
false) you must clear the auto-exposure history so a later enable doesn't see
garbage; update the bypass path in renderPass (where m_tonemapping is skipped)
to call a reset hook on the ToneMappingPass (e.g. add and call
m_tonemapping->reset_auto_exposure_history() or similar) or modify
ToneMappingPass::add_to_frame_graph to detect a newly-created
auto_exposure_result and zero-init it on first use; reference
m_tonemapping_enabled, m_tonemapping, ToneMappingPass::add_to_frame_graph and
auto_exposure_result when implementing the fix.
In `@core/src/rendering/renderWorld.cpp`:
- Around line 395-399: The HDR branch currently returns the raw pointer from
stbi_loadf_from_memory (called when has_hdr_extension(path) is true), but
callers free it with std::free, creating an allocator mismatch; fix by following
the LDR pattern: after calling stbi_loadf_from_memory, allocate a new buffer
with malloc of size (w * h * 4 * sizeof(float)), copy the pixels into that
malloc'd buffer, call stbi_image_free on the original stbi pointer, and return
the malloc'd pointer so callers can safely free it with std::free; reference
stbi_loadf_from_memory, stbi_image_free, and the has_hdr_extension(path) branch
when making the change.
- Around line 367-372: The loop currently only calls fb.insert for maps whose
channel exists (channels.findChannel(m.name)), leaving destination buffers for
missing EXR channels uninitialized and preventing Imf::Slice::fillValue from
being applied; change the loop in renderWorld.cpp to always call fb.insert for
every map (use fb.insert(m.name, Imf::Slice(Imf::FLOAT, base + m.offset *
sizeof(float), x_stride, y_stride, 1, 1, m.fill)) for each m in maps) so that
all RGBA slices are present in the FrameBuffer and their fillValue (m.fill) will
be applied even when the channel is absent in the file.
In `@core/src/rendering/toneMappingPass.cpp`:
- Around line 284-292: Replace use of global string keys fg.find("ssao") and
fg.find_or_create_buffer("auto_exposure_result") by making SSAO an explicit
input to this pass and by namespacing the persistent exposure buffer to the pass
instance; specifically, add an Inputs entry (e.g., Inputs::ssao or similar) and
use fg.find/input lookup instead of fg.find("ssao") so the SSAO dependency is
explicit, and change the buffer creation to use a namespaced key derived from
the pass instance (for example include the pass ID or name in the buffer key
used by find_or_create_buffer) instead of the raw global "auto_exposure_result"
string to avoid cross-instance aliasing and order-sensitivity.
In `@core/tests/testTextureResolution.cpp`:
- Around line 53-61: The file uses std::getenv inside the function
create_test_exr but does not directly include the header that declares it; add
an explicit include of <cstdlib> at the top of the file so create_test_exr and
any other uses of std::getenv are correctly declared across toolchains and avoid
relying on transitive includes.
In `@editor/src/editorApplication.cpp`:
- Around line 865-890: The EditorPass::render call is passing scene color/depth
handles that EditorPass doesn't use; remove the extra arguments at the call site
(replace m_editor_pass->render(*m_frame_graph, pass_ctx, scene_color_handle,
*scene_depth_handle) with m_editor_pass->render(*m_frame_graph, pass_ctx)) and
update the EditorPass::render declaration/definition (editorPass.h /
editorPass.cpp) to the new signature void
EditorPass::render(rendering::FrameGraph& fg, const rendering::PassContext& ctx)
so the unused color/depth parameters are removed; leave GridPass::render and
GridPass calls unchanged.
In `@editor/src/passes/editorPass.cpp`:
- Around line 175-176: The parameters color and depth in EditorPass::render are
declared but unused; either remove them from the signature if they are not
needed, or explicitly mark them as intentionally unused (e.g., cast to void or
add a /* reserved for future use */ comment) so the intent is clear; update the
EditorPass::render implementation and any callers accordingly to avoid
unused-parameter warnings and document that color/depth are reserved for future
integration.
In `@editor/src/passes/gridPass.cpp`:
- Around line 104-120: The BufferDesc and BindGroupDesc structs are not
value-initialized which can trigger static analysis warnings; update their
declarations to value-initialize them (e.g., change rendering::BufferDesc
buf_desc; to rendering::BufferDesc buf_desc{}; and rendering::BindGroupDesc
bg_desc; to rendering::BindGroupDesc bg_desc{};) while keeping the existing
assignments to buf_desc.size, buf_desc.usage, entry.binding, entry.buffer,
entry.buffer_size, bg_desc.layout, bg_desc.entries and the subsequent calls to
create_buffer and create_bind_group unchanged.
In `@editor/src/passes/lobePass.cpp`:
- Around line 100-116: The BufferDesc, BindGroupEntry, and BindGroupDesc structs
(buf_desc, entry, bg_desc) are flagged as potentially uninitialized; update
their declarations to value-initialize them (e.g., use {} initialization) before
setting fields so all padding and default members are zeroed, then keep the
existing assignments for size, usage, binding, buffer, buffer_size, layout, and
entries used by create_buffer and create_bind_group (referencing LobeUniforms
and ready.bind_group_layout).
In `@editor/tests/testAutoExposure.cpp`:
- Around line 62-71: Remove the local to_f16 lambda and use the shared utility
pts::rendering::float_to_half() instead: add the header that declares
float_to_half (core/rendering/halfFloat.h) to the includes at the top of the
test file, delete the lambda definition, and replace any uses of to_f16 with
pts::rendering::float_to_half(...) (or bring the symbol into scope) so the test
uses the canonical IEEE‑754 half conversion implementation.
In `@editor/tests/testLightGizmoVerts.cpp`:
- Around line 18-25: The test uses magic numbers for expected vertex counts;
replace those with named constants to improve clarity and maintainability by
defining constants (e.g., constexpr int CIRCLE_SEGMENTS = 48, CIRCLE_VERTS =
CIRCLE_SEGMENTS * 2, ARROW_SHAFT_VERTS = 2, ARROWHEAD_LINES = 4, ARROWHEAD_VERTS
= ARROWHEAD_LINES * 2, EXPECTED_TOTAL = CIRCLE_VERTS + ARROW_SHAFT_VERTS +
ARROWHEAD_VERTS) and use them in the assertion that checks verts.size() in the
TEST_CASE that constructs LightData and calls generate_light_verts so the intent
is explicit and easier to update if generate_light_verts changes.
In `@renderers/forward/forwardPass.cpp`:
- Around line 463-464: The PRECONDITION calls for shadow_out.shadow_array and
shadow_out.shadow_info assume shadows are always available; instead, check
whether the shadow pass is actually ready (e.g., call ShadowMapPass::is_ready()
or check a local flag that indicates shadow setup success) before enforcing
these PRECONDITIONs, and if the shadow pass is not ready, skip these assertions
and set the forward pass to use the non-shadow/fallback path (or log an error
and disable shadow usage) so the renderer degrades gracefully; locate uses of
PRECONDITION and the shadow_out struct in forwardPass.cpp
(shadow_out.shadow_array, shadow_out.shadow_info) and gate them behind the
readiness check or boolean that indicates ShadowMapPass success.
In `@renderers/pathtracer/pathtracer.slang`:
- Around line 120-185: The hit logic treats analytic light-proxy triangles the
same as emissive geometry, causing double-counting with NEE; update the
shading/hit handling so analytic light proxies are differentiated (e.g., add or
use a material flag like material.is_analytic_proxy or a light->is_proxy
property referenced by best.material_index / material lookup) and then when a
BSDF-sampled ray hits such a proxy either exclude the unconditional
mat.emissive_color contribution or apply the same MIS weight used for NEE
instead of adding the full emissive term; modify the code path that reads
mat.emissive_color after triangle hits (the place that uses best.material_index
and mat.emissive_color) to check that flag and skip or weight the emissive add
accordingly.
In `@renderers/pathtracer/pathTracerPass.cpp`:
- Around line 376-391: The local descriptor structs blit_buf_desc and
blit_bg_desc are not value-initialized which static analysis flags as
potentially uninitialized; to fix, value-initialize these structs where they are
declared (e.g., change the declarations for blit_buf_desc and blit_bg_desc to
use = {}), then keep the existing assignments to size, usage, layout, entries,
etc., so create_buffer(create_bind_group) calls (create_buffer with BlitUniforms
and create_bind_group with r.blit_bgl) receive fully-initialized descriptors and
future added fields default to zero.
---
Outside diff comments:
In `@core/src/rendering/renderWorld.cpp`:
- Around line 603-604: The TLAS rebuild condition only checks object
transform/mesh versions (need_rebuild uses any_blas_dirty, m_transform_version
vs m_cached_transform_version, and m_mesh_version vs m_cached_geometry_version)
but not light proxy changes; add a dedicated light-change signal (e.g.,
m_lights_version and m_cached_lights_version or any_lights_dirty) and include it
in the need_rebuild expression so changes to light position/visibility, proxy
mesh, or material (the code that appends visible lights to instances around the
visible-lights loop) will trigger TLAS/gpu_instances rebuilds; ensure whatever
code updates light proxies increments the new lights version so data.tlas and
gpu_instances are invalidated correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 70f4edd3-dd33-4dd4-bd55-e5dc711066a2
⛔ Files ignored due to path filters (15)
assets/models/camera/textures/Camera_01_body_diff_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_body_metallic_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_body_nor_gl_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_body_roughness_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_lens_body_diff_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_lens_body_metallic_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_lens_body_nor_gl_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_lens_body_roughness_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_strap_diff_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_strap_metallic_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_strap_nor_gl_2k.pngis excluded by!**/*.pngassets/models/camera/textures/Camera_01_strap_roughness_2k.pngis excluded by!**/*.pngassets/models/treasure_chest/textures/treasure_chest_diff_2k.jpgis excluded by!**/*.jpgconan_emscripten.lockis excluded by!**/*.lockconan_glfw.lockis excluded by!**/*.lock
📒 Files selected for processing (68)
.claude/skills/triage-ci/SKILL.md.github/workflows/ci.ymlCMakeLists.txtallowlist_extra.tomlassets/models/camera/Camera_01_2k.usdcassets/models/treasure_chest/textures/treasure_chest_metal_2k.exrassets/models/treasure_chest/textures/treasure_chest_nor_gl_2k.exrassets/models/treasure_chest/textures/treasure_chest_rough_2k.exrassets/models/treasure_chest/treasure_chest_2k.usdcassets/scenes/area_light_test.usdaassets/scenes/camera.usdaassets/scenes/shadow_test.usdaconanfile.pyconfig.yamlcore/CMakeLists.txtcore/include/core/rendering/aabb.hcore/include/core/rendering/frameGraph.hcore/include/core/rendering/gbufferPass.hcore/include/core/rendering/halfFloat.hcore/include/core/rendering/preparedSceneData.hcore/include/core/rendering/renderPass.hcore/include/core/rendering/renderWorld.hcore/include/core/rendering/renderer.hcore/include/core/rendering/shaderLoader.hcore/include/core/rendering/shadowMapPass.hcore/include/core/rendering/ssaoPass.hcore/include/core/rendering/toneMappingPass.hcore/shaders/shadow.slangcore/src/rendering/camera.cppcore/src/rendering/frameGraph.cppcore/src/rendering/gbufferPass.cppcore/src/rendering/iblResources.cppcore/src/rendering/ltcTextures.cppcore/src/rendering/renderPass.cppcore/src/rendering/renderWorld.cppcore/src/rendering/shaderLoader.cppcore/src/rendering/shadowMapPass.cppcore/src/rendering/ssaoPass.cppcore/src/rendering/toneMappingPass.cppcore/tests/CMakeLists.txtcore/tests/testFrameGraph.cppcore/tests/testMeshCache.cppcore/tests/testRendererRegistry.cppcore/tests/testShadowData.cppcore/tests/testShadowMapPass.cppcore/tests/testTextureResolution.cppeditor/shaders/luminance.slangeditor/shaders/tonemapping.slangeditor/src/editorApplication.cppeditor/src/include/editorApplication.heditor/src/passes/editorPass.cppeditor/src/passes/editorPass.heditor/src/passes/gridPass.cppeditor/src/passes/gridPass.heditor/src/passes/lobePass.cppeditor/src/passes/lobePass.heditor/src/passes/toneMappingPass.cppeditor/src/passes/wireframePass.cppeditor/src/passes/wireframePass.heditor/src/perfOverlay.heditor/tests/CMakeLists.txteditor/tests/testAutoExposure.cppeditor/tests/testLightGizmoVerts.cpprenderers/forward/forwardPass.cpprenderers/forward/forwardPass.hrenderers/pathtracer/pathTracerPass.cpprenderers/pathtracer/pathTracerPass.hrenderers/pathtracer/pathtracer.slang
💤 Files with no reviewable changes (2)
- core/tests/testShadowData.cpp
- editor/src/passes/toneMappingPass.cpp
Comment on lines
+115
to
117
| env: | ||
| PTSTUDIO_GPU_BACKEND: ${{ matrix.platform == 'windows-x64' && 'D3D12' || '' }} | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify how PTSTUDIO_GPU_BACKEND is consumed and whether empty means "unset" or "invalid override".
# 1) Find all readers/writers of the variable.
rg -n -C3 'PTSTUDIO_GPU_BACKEND'
# 2) Inspect backend selection logic for presence-vs-value checks.
rg -n -C4 'GPU_BACKEND|gpu backend|backend.*env|getenv|std::getenv|os\.environ|std::optional|FromString|parse.*backend'
# Expected:
# - Safe: logic ignores empty values (treats as unset/default).
# - Risky: logic branches only on variable presence and parses empty string.Repository: dw218192/PTStudio
Length of output: 41746
🏁 Script executed:
# Find and examine the workflow file at the specified lines
fd -t f 'ci.yml' .github/workflows/Repository: dw218192/PTStudio
Length of output: 85
🏁 Script executed:
# Examine the workflow file at the specified lines
head -200 .github/workflows/ci.yml | tail -100Repository: dw218192/PTStudio
Length of output: 3448
🏁 Script executed:
# Get exact line numbers and full context of lines 115-117 and 189-190
sed -n '110,120p' .github/workflows/ci.yml | cat -n
echo "---"
sed -n '185,195p' .github/workflows/ci.yml | cat -nRepository: dw218192/PTStudio
Length of output: 1049
Refactoring suggested: split conditional env var setup into platform-specific steps.
The current pattern PTSTUDIO_GPU_BACKEND: ${{ matrix.platform == 'windows-x64' && 'D3D12' || '' }} is functionally safe—the code at core/src/rendering/webgpu/device.cpp:156 uses std::getenv() and compares against known values, logging a warning for unknown/empty strings and falling back to default auto-selection. However, this generates a spurious warning log on every non-Windows run.
Refactoring into separate steps (Windows-only with if: matrix.platform == 'windows-x64') avoids the unnecessary warning and makes the intent clearer. Apply to both Build project and Run tests steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 115 - 117, Split the conditional
PTSTUDIO_GPU_BACKEND env assignment into platform-specific steps to avoid
emitting a spurious warning on non-Windows runs: remove the inline ternary-style
assignment PTSTUDIO_GPU_BACKEND: ${{ matrix.platform == 'windows-x64' && 'D3D12'
|| '' }} and instead create/modify the two workflow steps named "Build project"
and "Run tests" so each has a Windows-only variant guarded by if:
matrix.platform == 'windows-x64' that sets PTSTUDIO_GPU_BACKEND: D3D12, and
leave the non-Windows variants without that env var; ensure the steps still use
the same job matrix and runner so behavior is unchanged for Windows while
non-Windows runs no longer set an empty/unknown PTSTUDIO_GPU_BACKEND.
Comment on lines
+18
to
+20
| [[allow]] | ||
| name = "image_convert" | ||
| commands = ["ffmpeg"] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for ffmpeg usage in the codebase to understand necessity and scope.
# Search for ffmpeg references in code and scripts
echo "=== Searching for ffmpeg in code files ==="
rg -n -C3 'ffmpeg' --type-add 'code:*.{cpp,h,py,js,ts,sh,bat,ps1}' --type code
echo -e "\n=== Searching for ffmpeg in test files ==="
rg -n -C3 'ffmpeg' --glob '*test*'
echo -e "\n=== Searching for image conversion or visual regression related code ==="
rg -n -C3 'image.convert|visual.regression|regression.test' --type-add 'code:*.{cpp,h,py,js,ts,sh,bat}' --type codeRepository: dw218192/PTStudio
Length of output: 221
🏁 Script executed:
# Verify the file exists and show its full content
cat -n allowlist_extra.tomlRepository: dw218192/PTStudio
Length of output: 1091
🏁 Script executed:
# Search more broadly for any ffmpeg references (including comments and configs)
rg -n 'ffmpeg' -iRepository: dw218192/PTStudio
Length of output: 106
🏁 Script executed:
# Look for visual regression, EXR, image conversion infrastructure more broadly
rg -n '(visual|regression|EXR|openexr|image|texture|convert)' -i --max-count=20 | head -100Repository: dw218192/PTStudio
Length of output: 9811
🏁 Script executed:
# Check git log or PR description for context about ffmpeg
git log --oneline --all --grep='ffmpeg|image|visual' -i 2>/dev/null | head -20Repository: dw218192/PTStudio
Length of output: 43
Remove unrestricted ffmpeg allowlist entry—no corresponding code usage found.
The allowlist permits execution of any ffmpeg command, but a comprehensive search of the codebase reveals no calls to ffmpeg anywhere. This presents multiple concerns:
-
Security risk: Allowing unrestricted access to
ffmpeg(a powerful tool with known vulnerabilities) without corresponding code usage increases attack surface unnecessarily. -
Lack of justification: The PR objectives mention EXR texture support and visual regression testing, but the codebase uses
stb_image_writefor image output and texture APIs for handling—not CLI-basedffmpeg. -
Inconsistent patterns: Unlike
safe_git_rewriteandframework_contribwhich restrict commands withpatterns, this entry permits all operations without scope constraints. -
Missing usage: No code currently calls
ffmpeg, raising questions about whether this allowlist entry was added prematurely or by mistake.
Recommendation: Remove this allowlist entry unless there is immediate, documented need for ffmpeg with specific operations. If future use is planned, add it with restrictive patterns (specific input/output formats, read-only constraints) rather than unrestricted access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@allowlist_extra.toml` around lines 18 - 20, The allowlist contains an
unrestricted entry named "image_convert" that permits running "ffmpeg" but no
code references it; remove this unnecessary attack surface by deleting the
[[allow]] block with name "image_convert" and commands ["ffmpeg"] from
allowlist_extra.toml, or if ffmpeg is actually required in future, replace the
open command entry with a scoped allow that adds a restrictive "patterns" array
limiting executable arguments and file paths (e.g., explicit input/output file
patterns) instead of permitting all ffmpeg usage.
Comment on lines
+120
to
+121
| matrix4d xformOp:transform = ( (-0.5, 0, 0.8660253882408142, 0), (0.75, 0.5, 0.4330126941204071, 0), (-0.4330126941204071, 0.8660253882408142, -0.25, 0), (0, 5.531754016876221, 0, 1) ) | ||
| uniform token[] xformOpOrder = ["xformOp:transform"] |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Don't orphan the existing light rotation ops.
xformOpOrder now applies only xformOp:transform, so the remaining xformOp:rotateXYZ values on MainSun and FillSun are inert and easy to edit by mistake in tooling.
Also applies to: 130-131
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@assets/scenes/shadow_test.usda` around lines 120 - 121, The xformOpOrder
currently lists only "xformOp:transform", which leaves existing
xformOp:rotateXYZ entries on MainSun and FillSun orphaned and inert; update the
xformOpOrder to include the rotate ops in the correct order (e.g., include
"xformOp:rotateXYZ" entries alongside "xformOp:transform") for both MainSun and
FillSun so the rotate values are applied (also check and fix the same
xformOpOrder at the other occurrence noted around lines 130-131).
Comment on lines
+18
to
+25
| TEST_CASE("Distant light generates circle + arrow vertices") { | ||
| LightData light; | ||
| light.type = LightData::Type::Distant; | ||
| auto verts = generate_light_verts(light); | ||
|
|
||
| // 48 circle segments * 2 + arrow shaft (2) + arrowhead (4 lines * 2) = 106 | ||
| REQUIRE(verts.size() == 106); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider extracting magic numbers as named constants.
The vertex counts (48 segments, 96 circle verts, 106 total) are documented in comments but could be more maintainable as named constants, especially if generate_light_verts implementation changes.
🔧 Suggested improvement
+namespace {
+constexpr size_t k_circle_segments = 48;
+constexpr size_t k_circle_verts = k_circle_segments * 2;
+constexpr size_t k_arrow_verts = 2 + 4 * 2; // shaft + 4 arrowhead lines
+constexpr size_t k_distant_light_verts = k_circle_verts + k_arrow_verts;
+} // namespace
+
TEST_CASE("Distant light generates circle + arrow vertices") {
LightData light;
light.type = LightData::Type::Distant;
auto verts = generate_light_verts(light);
- // 48 circle segments * 2 + arrow shaft (2) + arrowhead (4 lines * 2) = 106
- REQUIRE(verts.size() == 106);
+ REQUIRE(verts.size() == k_distant_light_verts);
}🧰 Tools
🪛 Clang (14.0.6)
[warning] 18-18: variable 'TEST_CASE' is non-const and globally accessible, consider making it const
(cppcoreguidelines-avoid-non-const-global-variables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@editor/tests/testLightGizmoVerts.cpp` around lines 18 - 25, The test uses
magic numbers for expected vertex counts; replace those with named constants to
improve clarity and maintainability by defining constants (e.g., constexpr int
CIRCLE_SEGMENTS = 48, CIRCLE_VERTS = CIRCLE_SEGMENTS * 2, ARROW_SHAFT_VERTS = 2,
ARROWHEAD_LINES = 4, ARROWHEAD_VERTS = ARROWHEAD_LINES * 2, EXPECTED_TOTAL =
CIRCLE_VERTS + ARROW_SHAFT_VERTS + ARROWHEAD_VERTS) and use them in the
assertion that checks verts.size() in the TEST_CASE that constructs LightData
and calls generate_light_verts so the intent is explicit and easier to update if
generate_light_verts changes.
Comment on lines
+463
to
+464
| PRECONDITION(shadow_out.shadow_array.is_valid()); | ||
| PRECONDITION(shadow_out.shadow_info.is_valid()); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
PRECONDITION assumes shadow pass is always ready.
These preconditions will fail if ShadowMapPass isn't ready (e.g., setup failure or shader compilation error). This is acceptable if the forward renderer requires shadows, but consider if a fallback or more graceful degradation is desired.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@renderers/forward/forwardPass.cpp` around lines 463 - 464, The PRECONDITION
calls for shadow_out.shadow_array and shadow_out.shadow_info assume shadows are
always available; instead, check whether the shadow pass is actually ready
(e.g., call ShadowMapPass::is_ready() or check a local flag that indicates
shadow setup success) before enforcing these PRECONDITIONs, and if the shadow
pass is not ready, skip these assertions and set the forward pass to use the
non-shadow/fallback path (or log an error and disable shadow usage) so the
renderer degrades gracefully; locate uses of PRECONDITION and the shadow_out
struct in forwardPass.cpp (shadow_out.shadow_array, shadow_out.shadow_info) and
gate them behind the readiness check or boolean that indicates ShadowMapPass
success.
There was a problem hiding this comment.
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 `@tools/conan/dawn/conanfile.py`:
- Around line 42-44: The pip pin in conanfile.py (the self.run("pip install
\"jinja2<3.1.5\" --quiet") call) incorrectly cites removal of BaseLoader and
conflicts with the project's config.yaml which pins jinja2==3.1.6; investigate
the true incompatibility (look for Markup/escape/MarkupSafe usage in the Dawn
code generator), confirm whether the Dawn version being used requires a jinja2
<3.1.5 or has been patched, and then align the constraint with config.yaml or
document/justify the divergence; replace the bare pip install with an isolated
install approach (virtualenv, pipx, or conan/python-package helper) or move the
constraint into the recipe’s declared requirements so builds remain
reproducible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b357355c-b6bf-4de9-a417-48bf6c1b2772
📒 Files selected for processing (1)
tools/conan/dawn/conanfile.py
Comment on lines
+42
to
+44
| # Dawn's code generator requires jinja2 — pin a compatible version | ||
| # since jinja2 >=3.1.5 removed BaseLoader which Dawn still uses. | ||
| self.run("pip install \"jinja2<3.1.5\" --quiet") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does jinja2 3.1.5 or later remove BaseLoader? When was this change made and is there a workaround?
💡 Result:
No—Jinja2 3.1.5 does not remove BaseLoader, and it’s still re-exported from the top-level package (from jinja2 import BaseLoader) in 3.1.5. [1]
If you hit an “ImportError” around the same import line, it’s usually because Markup and escape were removed from jinja2 in Jinja 3.1.0 (they must be imported from MarkupSafe instead). This breaking change happened in Jinja 3.1.0 (released 2022-03-24) as part of “Remove previously deprecated code.” [2]
Workarounds / fixes
- Keep using
BaseLoaderas before (still valid):from jinja2 import BaseLoader[1]- or (more explicit / future-proof):
from jinja2.loaders import BaseLoader[1]
- If your failing import was something like
from jinja2 import BaseLoader, Markup, escape, change it to:from jinja2 import BaseLoaderfrom markupsafe import Markup, escape[2]
Extra context: 3.1.5 is described by the project as a security-fix release and “should not result in breaking changes compared to the latest feature release.” [3]
Sources:
[1] Jinja 3.1.5 jinja2/__init__.py re-exporting BaseLoader
[2] Jinja changelog: Version 3.1.0 removal of deprecated code; “Markup and escape should be imported from MarkupSafe.”
[3] Jinja 3.1.5 GitHub release notes statement about no breaking changes
Verify the actual jinja2 incompatibility and align version constraints.
The rationale in the comment—that jinja2 >=3.1.5 removed BaseLoader—is incorrect. BaseLoader is still available and re-exported in jinja2 3.1.5 and later. The actual breaking change in Jinja 3.1.0 was the removal of Markup and escape (which moved to MarkupSafe).
However, a real issue remains: the project's config.yaml specifies jinja2==3.1.6, while this recipe pins jinja2<3.1.5. These constraints conflict, and running this pip install will downgrade jinja2 in the active environment, potentially breaking other build steps expecting 3.1.6.
Before keeping this pin:
- Investigate what the actual jinja2 incompatibility is (likely related to Markup/escape usage, not BaseLoader)
- Verify whether the Dawn version in use actually has this incompatibility, or if it's been fixed upstream
- Align the version constraint with
config.yaml, or justify why they must differ
Avoid bare pip install without environment isolation—this can cause non-reproducible builds across different systems and environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/conan/dawn/conanfile.py` around lines 42 - 44, The pip pin in
conanfile.py (the self.run("pip install \"jinja2<3.1.5\" --quiet") call)
incorrectly cites removal of BaseLoader and conflicts with the project's
config.yaml which pins jinja2==3.1.6; investigate the true incompatibility (look
for Markup/escape/MarkupSafe usage in the Dawn code generator), confirm whether
the Dawn version being used requires a jinja2 <3.1.5 or has been patched, and
then align the constraint with config.yaml or document/justify the divergence;
replace the bare pip install with an isolated install approach (virtualenv,
pipx, or conan/python-package helper) or move the constraint into the recipe’s
declared requirements so builds remain reproducible.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tools/conan/dawn/conanfile.py (1)
43-71:⚠️ Potential issue | 🟠 MajorKeep Python and source dependency acquisition out of
build().Line 45 installs into whichever
pipis onPATH, while Line 53 runs whicheverpythonis onPATH, so the Jinja workaround can land in a different interpreter than the generator actually uses. More broadly, Lines 45, 53, and 63-64 makebuild()depend on ad-hoc installs/clones instead of Conan-managed inputs, which hurts reproducibility and offline builds. The BaseLoader rationale in Lines 43-44 also appears incorrect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tools/conan/dawn/conanfile.py` around lines 43 - 71, The build() method currently performs ad-hoc Python installs and repo manipulation (pip install jinja2, running fetch_dawn_dependencies.py, cloning abseil-cpp and removing third_party dirs) which makes builds non-reproducible and depends on whichever pip/python is on PATH; move all of that logic out of build() into appropriate Conan lifecycle methods: declare Jinja as a build-time Python requirement in build_requirements() or as a python_requires/build_requires entry so the correct interpreter/environment is used instead of calling pip in build(), run the fetch_dawn_dependencies.py and any Git clones/removals inside source() (use the Git helper/Git(self, ...) and existing fetch_script variable) so source acquisition is reproducible and offline-friendly, and stop installing via self.run("pip ...") and calling python from PATH — instead consume the declared build requirement/interpreter when invoking Python tools; update references in the code for fetch_script, third_party, abseil_dir, and Git(...) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tools/conan/dawn/conanfile.py`:
- Around line 51-53: The code currently skips running
tools/fetch_dawn_dependencies.py when it's missing, causing later
cmake.configure() failures; change the logic around fetch_script (computed from
self.source_folder) so that if DAWN_FETCH_DEPENDENCIES is disabled and
os.path.isfile(fetch_script) is False you raise immediately (e.g., raise
FileNotFoundError or ConanInvalidConfiguration) with a clear message about the
missing fetch helper instead of silently continuing; update the block that
references fetch_script/self.source_folder to perform this existence check and
raise when absent before attempting self.run().
---
Duplicate comments:
In `@tools/conan/dawn/conanfile.py`:
- Around line 43-71: The build() method currently performs ad-hoc Python
installs and repo manipulation (pip install jinja2, running
fetch_dawn_dependencies.py, cloning abseil-cpp and removing third_party dirs)
which makes builds non-reproducible and depends on whichever pip/python is on
PATH; move all of that logic out of build() into appropriate Conan lifecycle
methods: declare Jinja as a build-time Python requirement in
build_requirements() or as a python_requires/build_requires entry so the correct
interpreter/environment is used instead of calling pip in build(), run the
fetch_dawn_dependencies.py and any Git clones/removals inside source() (use the
Git helper/Git(self, ...) and existing fetch_script variable) so source
acquisition is reproducible and offline-friendly, and stop installing via
self.run("pip ...") and calling python from PATH — instead consume the declared
build requirement/interpreter when invoking Python tools; update references in
the code for fetch_script, third_party, abseil_dir, and Git(...) accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9795e40a-fba1-499e-b5bf-0689ef0921c0
📒 Files selected for processing (1)
tools/conan/dawn/conanfile.py
Comment on lines
+51
to
+53
| fetch_script = os.path.join(self.source_folder, "tools", "fetch_dawn_dependencies.py") | ||
| if os.path.isfile(fetch_script): | ||
| self.run(f"python \"{fetch_script}\" --directory \"{self.source_folder}\"") |
There was a problem hiding this comment.
Fail fast if the fetch helper is missing.
With DAWN_FETCH_DEPENDENCIES disabled on Line 31, silently skipping tools/fetch_dawn_dependencies.py here leaves cmake.configure() to fail later against a partial source tree. Raise immediately when the helper is absent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tools/conan/dawn/conanfile.py` around lines 51 - 53, The code currently skips
running tools/fetch_dawn_dependencies.py when it's missing, causing later
cmake.configure() failures; change the logic around fetch_script (computed from
self.source_folder) so that if DAWN_FETCH_DEPENDENCIES is disabled and
os.path.isfile(fetch_script) is False you raise immediately (e.g., raise
FileNotFoundError or ConanInvalidConfiguration) with a clear message about the
missing fetch helper instead of silently continuing; update the block that
references fetch_script/self.source_folder to perform this existence check and
raise when absent before attempting self.run().
dw218192
force-pushed
the
dev/rendering-next
branch
2 times, most recently
from
78fcdb4 to
272e06d
Compare
- Include light proxy meshes in BVH so the path tracer can hit and shade emitter geometry (area/sphere/disk lights) - Cap OpenEXR at <3.4 to avoid openjph → libtiff → xz_utils dep chain - Fix LDR texture gamma: use stbi_load (not stbi_loadf) for PNG/JPG to prevent double-linearization from stbi_loadf's sRGB conversion - Cache failed texture loads to avoid repeated decode attempts - Dawn recipe: manual dep fetch + patch Chromium abseil-cpp mirror, remove protobuf/libprotobuf-mutator, pin jinja2<3.1.5 - CI: CONAN_HOME on workspace drive (cross-drive full_deploy fix), bare prefix fallback for Conan cache restore
dw218192
force-pushed
the
dev/rendering-next
branch
from
272e06d to
ed8c84b
Compare
- Fix NaN→Inf corruption in float_to_half for small mantissa payloads - Fix zoom() clamping to [min_distance, max_distance] instead of near plane - Insert EXR slices unconditionally so missing channels get fill values - Use stbi_image_free for HDR path to match allocator contract - Replace strcmp dispatch in FrameGraph::make_pass_key with ResourceKind enum - Buffer reuse allows superset usage flags instead of exact match - Thread SSAO through ToneMappingPass::Inputs, scope exposure buffer per-pass - Add Material::light_index for proxy mesh MIS; skip emissive on analytic light proxies after bounce 0 to eliminate NEE double-counting - Drop inert xform ops in area_light_test.usda and shadow_test.usda - Remove unused LoggingManager forward decl, unused EditorPass params - Add [[nodiscard]] to ldr_output(), #include <cstdlib> in test - Value-initialize WebGPU descriptor structs in grid/lobe/pathtracer passes - Use float_to_half() utility in testAutoExposure instead of local lambda - Return type of BufferRef::size() changed to uint64_t
ensure_conan_profile() was hardcoded to ~/.conan2/profiles but CI sets CONAN_HOME=D:/.conan2 — the check found no profiles at the wrong path and ran conan profile detect, which failed because the profile already existed at the real CONAN_HOME.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
New Features
Improved Visuals
Bug Fixes
Documentation
Tests