refactor(render): normalize the filter-chain provider seam#130
Closed
refactor(render): normalize the filter-chain provider seam#130
Conversation
- Normalize provider resolution so render targets always link goggles-filter-chain - Preserve the host-versus-boundary ownership split for retarget and presentation - Keep backend code on the stable wrapper and public filter-chain headers - Add monorepo package rehearsal coverage for the consumer boundary - Archive and sync specs around the filter-chain consumer-boundary milestone
📝 WalkthroughWalkthroughThis PR introduces a normalized filter-chain consumer boundary through provider-resolution logic, reorganizes public headers under the Changes
Sequence DiagramsequenceDiagram
participant User as Developer/CI
participant CMake as CMake Configure
participant Provider as Provider Resolver
participant Build as Build System
participant Install as Package Install
participant Consumer as Consumer (find_package)
User->>CMake: cmake --preset smoke-provider-in-tree
CMake->>Provider: goggles_resolve_filter_chain_provider()
Provider->>Provider: Detect in-tree provider mode
Provider->>Provider: Scan/normalize in-tree targets
Provider->>Provider: Create goggles-filter-chain alias
Provider-->>CMake: Target ready
CMake->>Build: ninja -C build/smoke-provider-in-tree
Build-->>User: ✓ In-tree build complete
User->>CMake: cmake --preset smoke-provider-package-stage
CMake->>Provider: goggles_resolve_filter_chain_provider()
Provider->>Provider: Setup in-tree provider mode
Provider-->>CMake: Target ready
CMake->>Build: ninja -C build/smoke-provider-package-stage
Build->>Install: cmake --install
Install->>Install: Write GogglesFilterChainConfig.cmake
Install-->>User: ✓ Package installed to CMAKE_INSTALL_PREFIX
User->>CMake: cmake --preset smoke-provider-package-consumer
CMake->>Consumer: find_package(goggles-filter-chain)
Consumer->>Consumer: Load GogglesFilterChainConfig.cmake
Consumer->>Consumer: Validate Vulkan/spdlog/slang deps
Consumer->>Consumer: Resolve goggles-filter-chain target
Consumer-->>CMake: Consumer target configured
CMake->>Build: ninja -C build/smoke-provider-package-consumer
Build-->>User: ✓ Package consumer build complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
Review Summary by QodoNormalize filter-chain provider seam with boundary-owned public headers and diagnostic deduplication
WalkthroughsDescription• Normalize filter-chain provider resolution by establishing boundary-owned public headers under goggles/filter_chain/ for error handling, filter controls, scale modes, Vulkan context, and diagnostics • Move error types, result aliases, and filter control definitions from internal util/ and render/chain/ paths to boundary-owned public headers • Implement diagnostic event deduplication via state tracking in chain_executor and chain_resources to reduce redundant warnings • Update all internal code to use boundary-owned public headers instead of internal paths, maintaining stable wrapper and public filter-chain headers • Add comprehensive test coverage for filter-chain retarget contract, boundary header validation, and backend subsystem contracts • Refactor CMake configuration to normalize visibility (PUBLIC to PRIVATE) and add filter-chain include paths across render subsystems • Create CMake package configuration for goggles-filter-chain with proper dependency declarations • Add monorepo package rehearsal task for testing in-tree and package provider modes • Archive and sync specifications documenting filter-chain consumer-boundary milestone including build-system, render-pipeline, C API, and C++ wrapper contracts Diagramflowchart LR
A["Internal Paths<br/>util/ render/chain/"] -->|"Move to Boundary"| B["Public Headers<br/>goggles/filter_chain/"]
B -->|"Error & Result"| C["result.hpp<br/>error.hpp"]
B -->|"Types"| D["filter_controls.hpp<br/>vulkan_context.hpp<br/>scale_mode.hpp"]
B -->|"Diagnostics"| E["diagnostic_event.hpp<br/>diagnostic_session.hpp"]
F["Backend Code"] -->|"Use Boundary"| B
G["Chain Runtime"] -->|"Use Boundary"| B
H["Diagnostic Events"] -->|"Deduplicate via"| I["State Tracking<br/>semantic_keys<br/>binding_states"]
J["CMake Config"] -->|"Normalize"| K["Visibility<br/>Linkage<br/>Includes"]
File Changes1. src/render/chain/chain_executor.cpp
|
Code Review by Qodo
1. Provider uses undefined targets
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+80
to
+86
| add_library(goggles-filter-chain ${GOGGLES_CHAIN_LIBRARY_TYPE_NORMALIZED} | ||
| $<TARGET_OBJECTS:goggles_render_chain_obj> | ||
| $<TARGET_OBJECTS:goggles_render_shader_obj> | ||
| $<TARGET_OBJECTS:goggles_render_texture_obj> | ||
| $<TARGET_OBJECTS:goggles_diagnostics> | ||
| $<TARGET_OBJECTS:goggles_util_logging_obj> | ||
| ) |
There was a problem hiding this comment.
1. Provider uses undefined targets 🐞 Bug ✓ Correctness
The default in-tree provider path creates target goggles-filter-chain from OBJECT libraries goggles_diagnostics and goggles_util_logging_obj, but src/CMakeLists.txt processes src/render before src/util so those OBJECT targets do not exist when goggles_resolve_filter_chain_provider() runs, causing CMake configure to fail.
Agent Prompt
### Issue description
The default `in-tree` provider path configures `goggles-filter-chain` using `$<TARGET_OBJECTS:goggles_diagnostics>` and `$<TARGET_OBJECTS:goggles_util_logging_obj>` before those targets exist (because `src/util` is added after `src/render`). This breaks CMake configure for the default provider mode.
### Issue Context
`goggles_resolve_filter_chain_provider()` is invoked inside `src/render/CMakeLists.txt`, which is evaluated while processing `add_subdirectory(render)` from `src/CMakeLists.txt`.
### Fix Focus Areas
- src/CMakeLists.txt[3-7]
- src/render/CMakeLists.txt[1-23]
- cmake/GogglesFilterChainProvider.cmake[75-86]
- src/util/CMakeLists.txt[4-26]
- src/util/diagnostics/CMakeLists.txt[6-12]
### Suggested fix approach
- Move `add_subdirectory(util)` (and any other providers of `goggles_diagnostics`) before `add_subdirectory(render)` in `src/CMakeLists.txt`, OR
- Move the call site of `goggles_resolve_filter_chain_provider()` to a point after util/diagnostics targets are created, OR
- Stop depending on util/diagnostics OBJECT targets in `_goggles_filter_chain_enable_in_tree()` and instead link to normal libraries that are already defined at that point (if applicable).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
1
to
+3
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <cstdio> | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <nonstd/expected.hpp> | ||
| #include <source_location> | ||
| #include <string> | ||
| #include <utility> | ||
|
|
||
| namespace goggles { | ||
|
|
||
| /// @brief Error codes used by `goggles::Error`. | ||
| enum class ErrorCode : std::uint8_t { | ||
| ok, | ||
| file_not_found, | ||
| file_read_failed, | ||
| file_write_failed, | ||
| parse_error, | ||
| invalid_config, | ||
| vulkan_init_failed, | ||
| vulkan_device_lost, | ||
| shader_compile_failed, | ||
| shader_load_failed, | ||
| input_init_failed, | ||
| invalid_data, | ||
| unknown_error | ||
| }; | ||
|
|
||
| /// @brief Structured error for `Result<T>` operations. | ||
| struct Error { | ||
| ErrorCode code; | ||
| std::string message; | ||
| std::source_location location; | ||
|
|
||
| Error(ErrorCode error_code, std::string msg, | ||
| std::source_location loc = std::source_location::current()) | ||
| : code(error_code), message(std::move(msg)), location(loc) {} | ||
| }; | ||
|
|
||
| /// @brief Project-wide fallible operation return type. | ||
| template <typename T> | ||
| using Result = nonstd::expected<T, Error>; | ||
|
|
||
| /// @brief Convenience alias for `Result<std::unique_ptr<T>>`. | ||
| template <typename T> | ||
| using ResultPtr = Result<std::unique_ptr<T>>; | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto make_error(ErrorCode code, std::string message, | ||
| std::source_location loc = std::source_location::current()) | ||
| -> Result<T> { | ||
| return nonstd::make_unexpected(Error{code, std::move(message), loc}); | ||
| } | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto make_result_ptr(std::unique_ptr<T> ptr) -> ResultPtr<T> { | ||
| return ResultPtr<T>{std::move(ptr)}; | ||
| } | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto | ||
| make_result_ptr_error(ErrorCode code, std::string message, | ||
| std::source_location loc = std::source_location::current()) -> ResultPtr<T> { | ||
| return nonstd::make_unexpected(Error{code, std::move(message), loc}); | ||
| } | ||
|
|
||
| /// @brief Returns a stable string name for an `ErrorCode` value. | ||
| [[nodiscard]] constexpr auto error_code_name(ErrorCode code) -> const char* { | ||
| switch (code) { | ||
| case ErrorCode::ok: | ||
| return "ok"; | ||
| case ErrorCode::file_not_found: | ||
| return "file_not_found"; | ||
| case ErrorCode::file_read_failed: | ||
| return "file_read_failed"; | ||
| case ErrorCode::file_write_failed: | ||
| return "file_write_failed"; | ||
| case ErrorCode::parse_error: | ||
| return "parse_error"; | ||
| case ErrorCode::invalid_config: | ||
| return "invalid_config"; | ||
| case ErrorCode::vulkan_init_failed: | ||
| return "vulkan_init_failed"; | ||
| case ErrorCode::vulkan_device_lost: | ||
| return "vulkan_device_lost"; | ||
| case ErrorCode::shader_compile_failed: | ||
| return "shader_compile_failed"; | ||
| case ErrorCode::shader_load_failed: | ||
| return "shader_load_failed"; | ||
| case ErrorCode::input_init_failed: | ||
| return "input_init_failed"; | ||
| case ErrorCode::invalid_data: | ||
| return "invalid_data"; | ||
| case ErrorCode::unknown_error: | ||
| return "unknown_error"; | ||
| } | ||
| return "unknown"; | ||
| } | ||
|
|
||
| } // namespace goggles | ||
|
|
||
| // NOLINTBEGIN(cppcoreguidelines-macro-usage) | ||
|
|
||
| /// @brief Propagates an error or returns the contained value (expression-style). | ||
| /// | ||
| /// Similar to Rust's `?` operator. The expression must yield a `Result<T>`. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_TRY(expr) \ | ||
| ({ \ | ||
| auto _try_result = (expr); \ | ||
| if (!_try_result) \ | ||
| return nonstd::make_unexpected(_try_result.error()); \ | ||
| std::move(_try_result).value(); \ | ||
| }) | ||
|
|
||
| /// @brief Aborts on error or returns the contained value (expression-style). | ||
| /// | ||
| /// Use for internal invariants where failure indicates a bug. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_MUST(expr) \ | ||
| ({ \ | ||
| auto _must_result = (expr); \ | ||
| if (!_must_result) { \ | ||
| auto& _err = _must_result.error(); \ | ||
| std::fprintf(stderr, "GOGGLES_MUST failed at %s:%u in %s\n %s: %s\n", \ | ||
| _err.location.file_name(), _err.location.line(), \ | ||
| _err.location.function_name(), goggles::error_code_name(_err.code), \ | ||
| _err.message.c_str()); \ | ||
| std::abort(); \ | ||
| } \ | ||
| std::move(_must_result).value(); \ | ||
| }) | ||
|
|
||
| /// @brief Aborts when an invariant is violated. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_ASSERT(condition, ...) \ | ||
| do { \ | ||
| if (!(condition)) { \ | ||
| std::fprintf(stderr, "GOGGLES_ASSERT failed: %s at %s:%u in %s\n", #condition, \ | ||
| __FILE__, __LINE__, __func__); \ | ||
| __VA_OPT__(do { \ | ||
| std::fprintf(stderr, " "); \ | ||
| std::fprintf(stderr, __VA_ARGS__); \ | ||
| std::fprintf(stderr, "\n"); \ | ||
| } while (false);) \ | ||
| std::abort(); \ | ||
| } \ | ||
| } while (false) | ||
|
|
||
| // NOLINTEND(cppcoreguidelines-macro-usage) | ||
| #include "../render/chain/include/goggles/filter_chain/result.hpp" |
There was a problem hiding this comment.
2. Installed header includes source path 🐞 Bug ✓ Correctness
src/util/error.hpp is installed as a compatibility header but now includes result.hpp using a source-tree relative include (../render/chain/include/...), which will not exist in the install tree and will break consumers including <util/error.hpp>.
Agent Prompt
### Issue description
`src/util/error.hpp` is installed to `include/util/error.hpp` but includes `result.hpp` via a source-tree relative path (`../render/chain/include/...`). That path is invalid in the installed package layout, breaking downstream builds that rely on the compatibility include.
### Issue Context
The installed canonical header is `include/goggles/filter_chain/result.hpp`, and `util/error.hpp` should be a forwarder to that public surface.
### Fix Focus Areas
- src/util/error.hpp[1-3]
- src/render/CMakeLists.txt[56-80]
### Suggested fix approach
- Replace the include in `src/util/error.hpp` with:
- `#include <goggles/filter_chain/result.hpp>`
- (Optional) If you want to keep the forwarder strictly minimal, consider also adding a brief comment that it is a compatibility forwarder.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/util/CMakeLists.txt (1)
28-33:⚠️ Potential issue | 🟠 MajorMissing PIC setting on OBJECT library when building shared.
When
GOGGLES_CHAIN_LIBRARY_TYPEisSHARED, position-independent code is enabled ongoggles_util, butgoggles_util_logging_objis compiled before this check. Since OBJECT library objects are embedded directly into the final library, they also need PIC enabled, otherwise linking will fail on some platforms.Proposed fix
+if(DEFINED GOGGLES_CHAIN_LIBRARY_TYPE) + string(TOUPPER "${GOGGLES_CHAIN_LIBRARY_TYPE}" GOGGLES_CHAIN_LIBRARY_TYPE_NORMALIZED) + if(GOGGLES_CHAIN_LIBRARY_TYPE_NORMALIZED STREQUAL "SHARED") + set_target_properties(goggles_util_logging_obj PROPERTIES POSITION_INDEPENDENT_CODE ON) + endif() +endif() + add_library(goggles_util_logging_obj OBJECT logging.cpp )Alternatively, move the existing PIC block (lines 28-33) before the OBJECT library definition and apply it to both targets.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/CMakeLists.txt` around lines 28 - 33, When GOGGLES_CHAIN_LIBRARY_TYPE is SHARED you only set POSITION_INDEPENDENT_CODE on target goggles_util but not on the OBJECT library goggles_util_logging_obj (whose objects are embedded into the final library), causing link failures; fix by ensuring POSITION_INDEPENDENT_CODE is set for goggles_util_logging_obj as well when GOGGLES_CHAIN_LIBRARY_TYPE_NORMALIZED STREQUAL "SHARED" or move the entire PIC block that checks GOGGLES_CHAIN_LIBRARY_TYPE before the OBJECT library definition so it applies to both targets (ensure you reference the symbols GOGGLES_CHAIN_LIBRARY_TYPE, goggles_util, goggles_util_logging_obj, and the POSITION_INDEPENDENT_CODE property).
🧹 Nitpick comments (11)
src/util/CMakeLists.txt (1)
8-11:PUBLICon OBJECT library has no effect for object consumption.When
goggles_util_logging_objis consumed via$<TARGET_OBJECTS:...>(line 22), CMake does not propagate the OBJECT library's include directories or link dependencies. ThePUBLICkeyword here is effectively ignored. Consider usingPRIVATEto avoid misleading readers, since the actual propagation happens viagoggles_util's owntarget_include_directories.Suggested change
-target_include_directories(goggles_util_logging_obj PUBLIC +target_include_directories(goggles_util_logging_obj PRIVATE ${CMAKE_SOURCE_DIR}/src/render/chain/include ${CMAKE_CURRENT_SOURCE_DIR} )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/CMakeLists.txt` around lines 8 - 11, The target_include_directories call on the OBJECT library goggles_util_logging_obj uses PUBLIC which has no effect for OBJECT targets and is misleading; change the scope to PRIVATE (or remove the scope) so readers aren't misled and ensure the actual propagation is handled on the consuming target goggles_util (which uses $<TARGET_OBJECTS:goggles_util_logging_obj>); update the target_include_directories(goggles_util_logging_obj ...) invocation to use PRIVATE to clarify intent.openspec/specs/filter-chain-c-api/spec.md (1)
94-104: Minor formatting inconsistency in scenario blocks.The new scenario blocks use plain text for keywords (e.g.,
- GIVEN,- WHEN,- THEN), while existing scenarios in this spec use bold formatting (e.g.,- **GIVEN**,- **WHEN**,- **THEN**). Consider aligning with the existing convention for consistency.📝 Suggested formatting alignment
#### Scenario: Retarget preserves source-independent runtime state -- GIVEN a runtime has already completed preset load and is in READY state -- WHEN the host requests output-target retargeting for a format-only change through the C boundary -- THEN the C ABI contract SHALL preserve active preset identity and existing control state on success -- AND the host SHALL NOT need to reload the preset to adopt the new output target +- **GIVEN** a runtime has already completed preset load and is in READY state +- **WHEN** the host requests output-target retargeting for a format-only change through the C boundary +- **THEN** the C ABI contract SHALL preserve active preset identity and existing control state on success +- **AND** the host SHALL NOT need to reload the preset to adopt the new output targetApply the same pattern to the other new scenarios (lines 100-104, 112-117, 119-125).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openspec/specs/filter-chain-c-api/spec.md` around lines 94 - 104, Scenario blocks use plain keywords like "- GIVEN" instead of the spec's established bold format; update each scenario line in the new blocks to use bolded keywords (e.g., change "- GIVEN" to "- **GIVEN**", "- WHEN" to "- **WHEN**", "- THEN" to "- **THEN**") so the blocks for "Retarget preserves source-independent runtime state" and the subsequent related scenarios match the existing convention; apply the same replacement to the other new scenario lines mentioned in the comment (the blocks around lines 100-104, 112-117, 119-125) to keep formatting consistent.src/render/chain/include/goggles/filter_chain/scale_mode.hpp (1)
15-29: Consider adding an unreachable hint for safety.The
to_stringfunction handles all enum cases correctly and has a fallback return. However, some compilers may still emit warnings about control reaching the end of a non-void function. Consider moving the fallback into adefaultcase or adding a compiler hint if warnings arise.♻️ Alternative with default case
[[nodiscard]] constexpr auto to_string(ScaleMode mode) -> const char* { switch (mode) { case ScaleMode::fit: return "fit"; case ScaleMode::fill: return "fill"; case ScaleMode::stretch: return "stretch"; case ScaleMode::integer: return "integer"; case ScaleMode::dynamic: return "dynamic"; + default: + return "unknown"; } - return "unknown"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/include/goggles/filter_chain/scale_mode.hpp` around lines 15 - 29, The to_string(ScaleMode) switch should include an explicit default branch (instead of falling through after the switch) to silient compiler warnings and make intent clear: add a default case that returns "unknown" and, for safety, add an unreachable hint (e.g., call std::unreachable()/__builtin_unreachable() or assert(false)) after the return path so the compiler knows all enum cases are handled; update the to_string function's switch to include this default and the unreachable hint.tests/render/test_filter_chain_retarget_contract.cpp (1)
142-151: Consider makingCacheDirGuardnon-copyable and non-movable.This RAII guard manages a filesystem resource. Allowing copies could lead to double-deletion of the directory.
♻️ Proposed fix
struct CacheDirGuard { explicit CacheDirGuard(std::filesystem::path path) : dir(std::move(path)) {} ~CacheDirGuard() { if (!dir.empty()) { std::filesystem::remove_all(dir); } } + CacheDirGuard(const CacheDirGuard&) = delete; + CacheDirGuard& operator=(const CacheDirGuard&) = delete; + CacheDirGuard(CacheDirGuard&&) = delete; + CacheDirGuard& operator=(CacheDirGuard&&) = delete; std::filesystem::path dir; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_filter_chain_retarget_contract.cpp` around lines 142 - 151, CacheDirGuard currently allows implicit copying/moving which can cause double-deletion of the filesystem path; make CacheDirGuard non-copyable and non-movable by explicitly deleting the copy constructor, copy assignment, move constructor and move assignment (e.g. declare CacheDirGuard(const CacheDirGuard&) = delete; CacheDirGuard& operator=(const CacheDirGuard&) = delete; CacheDirGuard(CacheDirGuard&&) = delete; CacheDirGuard& operator=(CacheDirGuard&&) = delete;) while keeping the explicit CacheDirGuard(std::filesystem::path) constructor and the destructor as-is so the RAII semantics are preserved for dir.src/render/chain/include/goggles/filter_chain/filter_controls.hpp (1)
26-34: Consider usingdefaultcase for cleaner switch handling.The fallback return after the switch block handles invalid enum values but may trigger compiler warnings about control reaching end of non-void function in some configurations.
♻️ Proposed fix
[[nodiscard]] inline auto to_string(FilterControlStage stage) -> const char* { switch (stage) { case FilterControlStage::prechain: return "prechain"; case FilterControlStage::effect: return "effect"; + default: + return "effect"; } - return "effect"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/include/goggles/filter_chain/filter_controls.hpp` around lines 26 - 34, The to_string(FilterControlStage stage) function currently ends with a fallback return after the switch which can trigger warnings; replace that fallback with a default case inside the switch (e.g., default: return "effect";) so all control paths are covered, or alternatively add a default that logs/asserts on an invalid FilterControlStage before returning "effect", updating the switch in to_string to handle unexpected enum values directly.src/util/error.hpp (1)
3-3: Use angle-bracket includes for consistency with the rest of the codebase.The relative path is fragile and inconsistent. CMake already configures
${CMAKE_SOURCE_DIR}/src/render/chain/includeas a public include directory forgoggles_util, which enables angle-bracket includes like<goggles/filter_chain/result.hpp>. Files insrc/render/consistently use this style;src/util/should do the same.♻️ Proposed fix
`#pragma` once -#include "../render/chain/include/goggles/filter_chain/result.hpp" +#include <goggles/filter_chain/result.hpp>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/error.hpp` at line 3, Replace the fragile relative include in src/util/error.hpp with the project-standard angle-bracket include; specifically, change the current relative include of "../render/chain/include/goggles/filter_chain/result.hpp" to use <goggles/filter_chain/result.hpp> so it relies on the CMake public include directory and matches other files in src/render/.src/render/chain/api/c/goggles_filter_chain.cpp (1)
277-285: Consider addingdefaultcase annotation.The
to_api_stagefunction handles allFilterControlStageenum values, but the fallbackreturn GOGGLES_CHAIN_STAGE_EFFECTat line 284 is unreachable for a well-formed enum. Consider using[[unreachable]](C++23) or a comment/assert to clarify intent and help the compiler optimize.Optional improvement
auto to_api_stage(FilterControlStage stage) -> goggles_chain_stage_t { switch (stage) { case FilterControlStage::prechain: return GOGGLES_CHAIN_STAGE_PRECHAIN; case FilterControlStage::effect: return GOGGLES_CHAIN_STAGE_EFFECT; } - return GOGGLES_CHAIN_STAGE_EFFECT; + // Unreachable: all enum cases handled above + GOGGLES_ASSERT(false, "Unknown FilterControlStage value"); + return GOGGLES_CHAIN_STAGE_EFFECT; // Suppress warning }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/render/chain/api/c/goggles_filter_chain.cpp` around lines 277 - 285, The fallback return in to_api_stage(FilterControlStage stage) is intended to be unreachable; replace the bare fallback with an explicit unreachable annotation or assert to make intent clear and aid optimization: in function to_api_stage, after handling FilterControlStage::prechain and ::effect, either add a default case that calls std::unreachable() (or [[unreachable]] if available) or add an assertion like assert(false && "Unhandled FilterControlStage") before the final return, ensuring the compiler and readers know the code path is impossible.cmake/GogglesFilterChainProvider.cmake (2)
174-179: Use list length check instead of string comparison.CMake lists that are empty result in an empty string, but comparing with
STREQUAL ""can be fragile. Usinglist(LENGTH)is more idiomatic for CMake.♻️ Proposed fix
_goggles_filter_chain_candidate_targets(candidate_targets) - if(candidate_targets STREQUAL "") + list(LENGTH candidate_targets candidate_count) + if(candidate_count EQUAL 0) message(FATAL_ERROR "Filter-chain provider '${provider_mode}' did not expose a recognizable target. " "Expected goggles-filter-chain or a namespaced equivalent.") endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/GogglesFilterChainProvider.cmake` around lines 174 - 179, The empty-list check for candidate_targets using "STREQUAL \"\"" is fragile; update the logic in the _goggles_filter_chain_candidate_targets invocation to use CMake's list length check: call list(LENGTH candidate_targets <var>) and test if that length EQUAL 0, then call message(FATAL_ERROR ...) with the same provider_mode message; ensure you reference the existing variable name candidate_targets and provider_mode and replace the STREQUAL "" branch with the list(LENGTH) check.
42-50: TheBUILDSYSTEM_TARGETScheck is redundant for subdirectory targets.The
if(TARGET)check at lines 35-40 already captures targets from any scope (including subdirectories added viaadd_subdirectoryat line 167-169) using the known candidate names. TheBUILDSYSTEM_TARGETSproperty at line 42 only contains targets created directly in the current directory—it excludes targets from subdirectories, making the regex pattern at line 44 unlikely to find additional targets beyond what the first block already found.Consider whether the second block adds value or can be removed, since the candidate list in lines 25-33 already covers the expected naming patterns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/GogglesFilterChainProvider.cmake` around lines 42 - 50, Remove the redundant BUILDSYSTEM_TARGETS fallback block: delete the get_property(global_targets DIRECTORY PROPERTY BUILDSYSTEM_TARGETS) ... list(REMOVE_DUPLICATES found_targets) set(${out_targets} "${found_targets}" PARENT_SCOPE) sequence, since the earlier if(TARGET ...) candidate checks already find targets (including those from add_subdirectory) and the BUILDSYSTEM_TARGETS property only contains targets created in the current directory; keep the existing found_targets accumulation and final out_targets export produced by the first block (the if(TARGET) logic) so behavior is unchanged.cmake/GogglesFilterChainConfig.cmake.in (1)
9-17: CONDA_PREFIX dependency may limit portability.The stb_image fallback requires
CONDA_PREFIXto be set, which restricts package consumption to conda environments. If broader package distribution is planned, consider:
- Using
find_pathwith standard search paths as a fallback- Documenting this requirement in the package README
For monorepo rehearsal purposes as stated in the design doc, this is acceptable, but flagging for awareness if standalone package readiness becomes a goal.
💡 Suggested alternative for broader portability
if(NOT TARGET stb_image) - if(NOT DEFINED ENV{CONDA_PREFIX}) - message(FATAL_ERROR "CONDA_PREFIX must be set to consume goggles-filter-chain") - endif() + find_path(STB_IMAGE_INCLUDE_DIR + NAMES stb_image.h + PATHS + "$ENV{CONDA_PREFIX}/include/stb" + "/usr/include/stb" + "/usr/local/include/stb" + ) + if(NOT STB_IMAGE_INCLUDE_DIR) + message(FATAL_ERROR "stb_image.h not found. Set STB_IMAGE_INCLUDE_DIR or ensure stb is installed.") + endif() add_library(stb_image INTERFACE IMPORTED) set_target_properties(stb_image PROPERTIES - INTERFACE_INCLUDE_DIRECTORIES "$ENV{CONDA_PREFIX}/include/stb" + INTERFACE_INCLUDE_DIRECTORIES "${STB_IMAGE_INCLUDE_DIR}" ) endif()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmake/GogglesFilterChainConfig.cmake.in` around lines 9 - 17, The current fallback that requires ENV{CONDA_PREFIX} for the stb_image INTERFACE target (add_library(stb_image INTERFACE IMPORTED) + set_target_properties(... INTERFACE_INCLUDE_DIRECTORIES "$ENV{CONDA_PREFIX}/include/stb")) limits portability; change the CMake logic to first try a generic find_path for the stb headers (use find_path to locate e.g. "stb_image.h" with common system and project include locations), and only if find_path fails fall back to using ENV{CONDA_PREFIX} to set INTERFACE_INCLUDE_DIRECTORIES, keeping the add_library(stb_image INTERFACE IMPORTED) and set_target_properties usage; also add/update README to document the CONDA_PREFIX fallback requirement so consumers know about the environment dependency.tests/render/test_filter_boundary_contracts.cpp (1)
22-28: Consider extractingread_text_fileto a shared test utility.This helper function is duplicated identically in
test_vulkan_backend_subsystem_contracts.cpp(lines 20-26) andtest_filter_chain.cpp(lines 74-80). Extracting it to a shared test utility header would reduce duplication.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/render/test_filter_boundary_contracts.cpp` around lines 22 - 28, The helper function read_text_file is duplicated across multiple tests; extract it into a shared test utility header (declare and implement the function once, either inline in the header or in a small test-only source) and replace the duplicate copies in test_filter_boundary_contracts.cpp, test_vulkan_backend_subsystem_contracts.cpp and test_filter_chain.cpp with an `#include` of that shared header and calls to read_text_file; ensure the signature and behavior (returns std::optional<std::string>) remain identical and update includes (e.g., <fstream>, <filesystem>, <optional>, <string>) in the shared header so test files don’t need duplicated includes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CMakePresets.json`:
- Around line 222-232: The build preset "smoke-provider-package-stage" sets
CMAKE_INSTALL_PREFIX but doesn't run the install step, so running cmake --build
--preset smoke-provider-package-stage won't populate the install tree; update
the "smoke-provider-package-stage" build preset to include the install target
(e.g. add "targets": ["install"] or equivalent) so the configured
CMAKE_INSTALL_PREFIX is populated, and apply the same change to the other
analogous preset referenced in the comment (lines 306-309) to ensure both
package-stage presets run install.
In `@src/render/chain/chain_executor.cpp`:
- Around line 231-243: The current logic freezes semantic records on first sight
by using resources.m_recorded_semantic_keys (semantic_record_key(pass_ordinal,
member.name)) to gate session.record_semantic, which causes stale live-derived
values (e.g., semantic_value fields like FrameCount and alias-backed *Size
members) to never update after the first frame/resize; instead, either move the
presence check to only suppress duplicate warnings (i.e., keep the
first-observation gate for warning emission but always call
session.record_semantic), or implement value-based change detection: store the
last recorded semantic_value per (pass_ordinal, member.name) and compare the
current semantic_value (including FrameCount and size fields) before calling
session.record_semantic only when it has changed; update
resources.m_recorded_semantic_keys usage accordingly and ensure any alias-backed
size fields are evaluated after bindings are prepared before
comparing/recording.
In `@src/render/chain/include/goggles/filter_chain/error.hpp`:
- Line 8: The header declares types under namespace goggles but lives in
goggles/filter_chain, so update the namespace to goggles::filter_chain for the
public types (Error, ErrorCode, and any related declarations) to match the
directory boundary; change the opening namespace declaration from namespace
goggles to namespace goggles::filter_chain and update the matching closing
comment/brace accordingly so symbols are emitted as goggles::filter_chain::Error
and goggles::filter_chain::ErrorCode.
- Line 4: The public header uses std::source_location (requires C++20) but the
installed CMake config template doesn't enforce C++20 for consumers; update the
GogglesFilterChainConfig.cmake.in template so that both the exported target
(GogglesFilterChain::goggles-filter-chain) and the imported legacy target
(goggles-filter-chain) get INTERFACE_COMPILE_FEATURES "cxx_std_20" added when
the targets are defined/installed, ensuring consumers compile with C++20
compatibility for symbols like std::source_location used in
src/render/chain/include/goggles/filter_chain/error.hpp.
In `@src/render/chain/include/goggles/filter_chain/result.hpp`:
- Around line 5-9: The file is missing the <source_location> header used by
std::source_location in the factory functions make_error and
make_result_ptr_error; fix by adding an `#include` <source_location> to the
includes at the top of result.hpp so the symbols used by make_error and
make_result_ptr_error compile correctly with all compilers.
In `@src/render/shader/CMakeLists.txt`:
- Around line 25-28: The compile definitions for the target are currently
applied as PRIVATE which causes downstream consumers including slang_reflect.hpp
(exposed via target_include_directories(PUBLIC)) to be built without
VULKAN_HPP_NO_EXCEPTIONS and VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1, risking
ODR/ABI mismatches; change the call to
target_compile_definitions(goggles_render_shader_obj ...) so the definitions
VULKAN_HPP_NO_EXCEPTIONS and VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1 are PUBLIC
(not PRIVATE) so that consumers including <vulkan/vulkan.hpp> get the same macro
settings as the library.
In `@src/util/CMakeLists.txt`:
- Line 36: The util CMakeLists should not include render/chain headers because
Error, ErrorCode, and Result<T> are causing a circular dependency; extract those
types into a shared utility module (e.g., goggles_types or move them directly
into util) and update CMake and includes accordingly: create a new
target/library that defines Error, ErrorCode, and Result<T> (or move their
definitions into util headers referenced by util/config.hpp), change
render/chain and util to depend on that new target instead of render/chain, and
remove `${CMAKE_SOURCE_DIR}/src/render/chain/include` from util's include list
so the circular link is eliminated.
In `@src/util/config.hpp`:
- Line 3: The include in src/util/config.hpp currently uses a repo-relative path
to scale_mode.hpp
("../render/chain/include/goggles/filter_chain/scale_mode.hpp"); change it to
the public filter-chain include (e.g. use the stable public header path such as
"goggles/filter_chain/scale_mode.hpp" or "filter_chain/scale_mode.hpp" depending
on the project's public include root) so the header is resolved via the
installed/public include contract; make the equivalent change in
src/util/error.hpp where the same repo-relative include appears so both files
consume the public filter-chain header instead of hardcoded source-tree paths.
In `@tests/CMakeLists.txt`:
- Around line 144-146: The test target goggles_filter_chain_contract_tests is
being registered twice (via add_test and via catch_discover_tests), causing
duplicate runs and lost ASAN/label properties; fix by removing or guarding the
add_test registration when Catch.cmake discovery is used (i.e., only call
add_test if catch_discover_tests is not available) OR remove add_test and
instead pass the ASAN and label properties to catch_discover_tests using its
PROPERTIES argument so the discovered individual cases inherit the same
properties (apply to the goggles_filter_chain_contract_tests target and the
existing ASAN/label property names used elsewhere).
In `@tests/render/test_filter_chain_retarget_contract.cpp`:
- Around line 90-99: Remove the redundant assignment of
features13.dynamicRendering after the feature check and correctly build the
pNext chain so the base features struct is included: set features2.pNext to
&features13 and then assign device_info.pNext = &features2 (instead of
&features13) so the chain begins with features2 and continues to features13;
keep the existing check that returns if features13.dynamicRendering != VK_TRUE
but do not reassign it.
---
Outside diff comments:
In `@src/util/CMakeLists.txt`:
- Around line 28-33: When GOGGLES_CHAIN_LIBRARY_TYPE is SHARED you only set
POSITION_INDEPENDENT_CODE on target goggles_util but not on the OBJECT library
goggles_util_logging_obj (whose objects are embedded into the final library),
causing link failures; fix by ensuring POSITION_INDEPENDENT_CODE is set for
goggles_util_logging_obj as well when GOGGLES_CHAIN_LIBRARY_TYPE_NORMALIZED
STREQUAL "SHARED" or move the entire PIC block that checks
GOGGLES_CHAIN_LIBRARY_TYPE before the OBJECT library definition so it applies to
both targets (ensure you reference the symbols GOGGLES_CHAIN_LIBRARY_TYPE,
goggles_util, goggles_util_logging_obj, and the POSITION_INDEPENDENT_CODE
property).
---
Nitpick comments:
In `@cmake/GogglesFilterChainConfig.cmake.in`:
- Around line 9-17: The current fallback that requires ENV{CONDA_PREFIX} for the
stb_image INTERFACE target (add_library(stb_image INTERFACE IMPORTED) +
set_target_properties(... INTERFACE_INCLUDE_DIRECTORIES
"$ENV{CONDA_PREFIX}/include/stb")) limits portability; change the CMake logic to
first try a generic find_path for the stb headers (use find_path to locate e.g.
"stb_image.h" with common system and project include locations), and only if
find_path fails fall back to using ENV{CONDA_PREFIX} to set
INTERFACE_INCLUDE_DIRECTORIES, keeping the add_library(stb_image INTERFACE
IMPORTED) and set_target_properties usage; also add/update README to document
the CONDA_PREFIX fallback requirement so consumers know about the environment
dependency.
In `@cmake/GogglesFilterChainProvider.cmake`:
- Around line 174-179: The empty-list check for candidate_targets using
"STREQUAL \"\"" is fragile; update the logic in the
_goggles_filter_chain_candidate_targets invocation to use CMake's list length
check: call list(LENGTH candidate_targets <var>) and test if that length EQUAL
0, then call message(FATAL_ERROR ...) with the same provider_mode message;
ensure you reference the existing variable name candidate_targets and
provider_mode and replace the STREQUAL "" branch with the list(LENGTH) check.
- Around line 42-50: Remove the redundant BUILDSYSTEM_TARGETS fallback block:
delete the get_property(global_targets DIRECTORY PROPERTY BUILDSYSTEM_TARGETS)
... list(REMOVE_DUPLICATES found_targets) set(${out_targets} "${found_targets}"
PARENT_SCOPE) sequence, since the earlier if(TARGET ...) candidate checks
already find targets (including those from add_subdirectory) and the
BUILDSYSTEM_TARGETS property only contains targets created in the current
directory; keep the existing found_targets accumulation and final out_targets
export produced by the first block (the if(TARGET) logic) so behavior is
unchanged.
In `@openspec/specs/filter-chain-c-api/spec.md`:
- Around line 94-104: Scenario blocks use plain keywords like "- GIVEN" instead
of the spec's established bold format; update each scenario line in the new
blocks to use bolded keywords (e.g., change "- GIVEN" to "- **GIVEN**", "- WHEN"
to "- **WHEN**", "- THEN" to "- **THEN**") so the blocks for "Retarget preserves
source-independent runtime state" and the subsequent related scenarios match the
existing convention; apply the same replacement to the other new scenario lines
mentioned in the comment (the blocks around lines 100-104, 112-117, 119-125) to
keep formatting consistent.
In `@src/render/chain/api/c/goggles_filter_chain.cpp`:
- Around line 277-285: The fallback return in to_api_stage(FilterControlStage
stage) is intended to be unreachable; replace the bare fallback with an explicit
unreachable annotation or assert to make intent clear and aid optimization: in
function to_api_stage, after handling FilterControlStage::prechain and ::effect,
either add a default case that calls std::unreachable() (or [[unreachable]] if
available) or add an assertion like assert(false && "Unhandled
FilterControlStage") before the final return, ensuring the compiler and readers
know the code path is impossible.
In `@src/render/chain/include/goggles/filter_chain/filter_controls.hpp`:
- Around line 26-34: The to_string(FilterControlStage stage) function currently
ends with a fallback return after the switch which can trigger warnings; replace
that fallback with a default case inside the switch (e.g., default: return
"effect";) so all control paths are covered, or alternatively add a default that
logs/asserts on an invalid FilterControlStage before returning "effect",
updating the switch in to_string to handle unexpected enum values directly.
In `@src/render/chain/include/goggles/filter_chain/scale_mode.hpp`:
- Around line 15-29: The to_string(ScaleMode) switch should include an explicit
default branch (instead of falling through after the switch) to silient compiler
warnings and make intent clear: add a default case that returns "unknown" and,
for safety, add an unreachable hint (e.g., call
std::unreachable()/__builtin_unreachable() or assert(false)) after the return
path so the compiler knows all enum cases are handled; update the to_string
function's switch to include this default and the unreachable hint.
In `@src/util/CMakeLists.txt`:
- Around line 8-11: The target_include_directories call on the OBJECT library
goggles_util_logging_obj uses PUBLIC which has no effect for OBJECT targets and
is misleading; change the scope to PRIVATE (or remove the scope) so readers
aren't misled and ensure the actual propagation is handled on the consuming
target goggles_util (which uses $<TARGET_OBJECTS:goggles_util_logging_obj>);
update the target_include_directories(goggles_util_logging_obj ...) invocation
to use PRIVATE to clarify intent.
In `@src/util/error.hpp`:
- Line 3: Replace the fragile relative include in src/util/error.hpp with the
project-standard angle-bracket include; specifically, change the current
relative include of "../render/chain/include/goggles/filter_chain/result.hpp" to
use <goggles/filter_chain/result.hpp> so it relies on the CMake public include
directory and matches other files in src/render/.
In `@tests/render/test_filter_boundary_contracts.cpp`:
- Around line 22-28: The helper function read_text_file is duplicated across
multiple tests; extract it into a shared test utility header (declare and
implement the function once, either inline in the header or in a small test-only
source) and replace the duplicate copies in test_filter_boundary_contracts.cpp,
test_vulkan_backend_subsystem_contracts.cpp and test_filter_chain.cpp with an
`#include` of that shared header and calls to read_text_file; ensure the signature
and behavior (returns std::optional<std::string>) remain identical and update
includes (e.g., <fstream>, <filesystem>, <optional>, <string>) in the shared
header so test files don’t need duplicated includes.
In `@tests/render/test_filter_chain_retarget_contract.cpp`:
- Around line 142-151: CacheDirGuard currently allows implicit copying/moving
which can cause double-deletion of the filesystem path; make CacheDirGuard
non-copyable and non-movable by explicitly deleting the copy constructor, copy
assignment, move constructor and move assignment (e.g. declare
CacheDirGuard(const CacheDirGuard&) = delete; CacheDirGuard& operator=(const
CacheDirGuard&) = delete; CacheDirGuard(CacheDirGuard&&) = delete;
CacheDirGuard& operator=(CacheDirGuard&&) = delete;) while keeping the explicit
CacheDirGuard(std::filesystem::path) constructor and the destructor as-is so the
RAII semantics are preserved for dir.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 823c7b1d-4ff8-42a6-a89d-9a2499c6b039
📒 Files selected for processing (73)
CMakePresets.jsoncmake/GogglesFilterChainConfig.cmake.incmake/GogglesFilterChainProvider.cmakeopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/design.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/proposal.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/build-system/spec.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/filter-chain-c-api/spec.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/filter-chain-cpp-wrapper/spec.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/goggles-filter-chain/spec.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/render-pipeline/spec.mdopenspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/tasks.mdopenspec/specs/build-system/spec.mdopenspec/specs/filter-chain-c-api/spec.mdopenspec/specs/filter-chain-cpp-wrapper/spec.mdopenspec/specs/goggles-filter-chain/spec.mdopenspec/specs/render-pipeline/spec.mdpixi.tomlscripts/task/help.shscripts/task/rehearse-filter-chain-provider.shsrc/render/CMakeLists.txtsrc/render/backend/CMakeLists.txtsrc/render/backend/external_frame_importer.hppsrc/render/backend/filter_chain_controller.cppsrc/render/backend/filter_chain_controller.hppsrc/render/backend/render_output.hppsrc/render/backend/vulkan_backend.cppsrc/render/backend/vulkan_backend.hppsrc/render/backend/vulkan_context.hppsrc/render/backend/vulkan_debug.hppsrc/render/backend/vulkan_error.hppsrc/render/chain/CMakeLists.txtsrc/render/chain/api/c/goggles_filter_chain.cppsrc/render/chain/api/c/goggles_filter_chain.hsrc/render/chain/api/cpp/goggles_filter_chain.hppsrc/render/chain/chain_builder.cppsrc/render/chain/chain_builder.hppsrc/render/chain/chain_executor.cppsrc/render/chain/chain_executor.hppsrc/render/chain/chain_resources.cppsrc/render/chain/chain_resources.hppsrc/render/chain/chain_runtime.cppsrc/render/chain/chain_runtime.hppsrc/render/chain/filter_controls.hppsrc/render/chain/filter_pass.hppsrc/render/chain/framebuffer.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/compile_report.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_event.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_policy.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_session.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_sink.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/gpu_timestamp_pool.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/log_sink.hppsrc/render/chain/include/goggles/filter_chain/diagnostics/source_provenance.hppsrc/render/chain/include/goggles/filter_chain/error.hppsrc/render/chain/include/goggles/filter_chain/filter_controls.hppsrc/render/chain/include/goggles/filter_chain/result.hppsrc/render/chain/include/goggles/filter_chain/scale_mode.hppsrc/render/chain/include/goggles/filter_chain/vulkan_context.hppsrc/render/chain/pass.hppsrc/render/chain/preset_parser.hppsrc/render/chain/vulkan_context.hppsrc/render/chain/vulkan_result.hppsrc/render/shader/CMakeLists.txtsrc/render/shader/retroarch_preprocessor.hppsrc/render/shader/shader_runtime.hppsrc/render/texture/CMakeLists.txtsrc/util/CMakeLists.txtsrc/util/config.hppsrc/util/error.hpptests/CMakeLists.txttests/render/test_filter_boundary_contracts.cpptests/render/test_filter_chain_retarget_contract.cpptests/render/test_vulkan_backend_subsystem_contracts.cpp
Comment on lines
+222
to
+232
| "name": "smoke-provider-package-stage", | ||
| "displayName": "Provider rehearsal package stage", | ||
| "description": "Build and install the filter-chain package for provider rehearsal", | ||
| "inherits": [ | ||
| ".debug", | ||
| ".test" | ||
| ], | ||
| "cacheVariables": { | ||
| "GOGGLES_FILTER_CHAIN_PROVIDER": "in-tree", | ||
| "CMAKE_INSTALL_PREFIX": "${sourceDir}/build/smoke-provider-package-root" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's get the size and content of CMakePresets.json
wc -l CMakePresets.jsonRepository: goggles-dev/Goggles
Length of output: 84
🏁 Script executed:
# Read the section around lines 222-232 to see the configure preset
sed -n '210,240p' CMakePresets.jsonRepository: goggles-dev/Goggles
Length of output: 1092
🏁 Script executed:
# Also check the build preset and lines 306-309 for the "Also applies to" reference
sed -n '290,320p' CMakePresets.jsonRepository: goggles-dev/Goggles
Length of output: 898
🏁 Script executed:
# Let's also see the overall structure of the file to understand the preset organization
grep -n '"name"' CMakePresets.json | grep -E 'smoke-provider|provider-package'Repository: goggles-dev/Goggles
Length of output: 453
🏁 Script executed:
# Get more complete picture of the build preset structure
sed -n '305,315p' CMakePresets.jsonRepository: goggles-dev/Goggles
Length of output: 328
🏁 Script executed:
# Check if there are any test presets that might build or install
sed -n '340,360p' CMakePresets.jsonRepository: goggles-dev/Goggles
Length of output: 773
Add install target to the package-stage build preset.
The smoke-provider-package-stage configure preset sets CMAKE_INSTALL_PREFIX, but the build preset only builds the default target without running install. Running cmake --build --preset smoke-provider-package-stage will not populate build/smoke-provider-package-root, leaving nothing for the package-consumer preset to resolve.
Fix
{
"name": "smoke-provider-package-stage",
- "configurePreset": "smoke-provider-package-stage"
+ "configurePreset": "smoke-provider-package-stage",
+ "targets": ["install"]
},Also applies to: 306-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CMakePresets.json` around lines 222 - 232, The build preset
"smoke-provider-package-stage" sets CMAKE_INSTALL_PREFIX but doesn't run the
install step, so running cmake --build --preset smoke-provider-package-stage
won't populate the install tree; update the "smoke-provider-package-stage" build
preset to include the install target (e.g. add "targets": ["install"] or
equivalent) so the configured CMAKE_INSTALL_PREFIX is populated, and apply the
same change to the other analogous preset referenced in the comment (lines
306-309) to ensure both package-stage presets run install.
Comment on lines
+231
to
+243
| const auto semantic_key = semantic_record_key(pass_ordinal, member.name); | ||
| const bool first_observation = | ||
| resources.m_recorded_semantic_keys.insert(semantic_key).second; | ||
|
|
||
| if (first_observation) { | ||
| session.record_semantic({ | ||
| .pass_ordinal = pass_ordinal, | ||
| .member_name = member.name, | ||
| .classification = classification, | ||
| .value = value, | ||
| .offset = static_cast<uint32_t>(member.offset), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Don't freeze semantic records on first sight.
session.record_semantic() now runs only once per pass/member, but semantic_value() carries live data (FrameCount, size semantics) and some alias-backed *Size members are only correct after bindings are prepared. After the first frame, or after a resize, the semantic ledger will keep stale values forever. If you only want to suppress duplicate warnings, keep the first-observation gate on the warning path, or switch this to value-based change detection instead of a presence set.
Minimal correctness fix
- if (first_observation) {
- session.record_semantic({
- .pass_ordinal = pass_ordinal,
- .member_name = member.name,
- .classification = classification,
- .value = value,
- .offset = static_cast<uint32_t>(member.offset),
- });
- }
+ session.record_semantic({
+ .pass_ordinal = pass_ordinal,
+ .member_name = member.name,
+ .classification = classification,
+ .value = value,
+ .offset = static_cast<uint32_t>(member.offset),
+ });📝 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.
Suggested change
| const auto semantic_key = semantic_record_key(pass_ordinal, member.name); | |
| const bool first_observation = | |
| resources.m_recorded_semantic_keys.insert(semantic_key).second; | |
| if (first_observation) { | |
| session.record_semantic({ | |
| .pass_ordinal = pass_ordinal, | |
| .member_name = member.name, | |
| .classification = classification, | |
| .value = value, | |
| .offset = static_cast<uint32_t>(member.offset), | |
| }); | |
| } | |
| const auto semantic_key = semantic_record_key(pass_ordinal, member.name); | |
| const bool first_observation = | |
| resources.m_recorded_semantic_keys.insert(semantic_key).second; | |
| session.record_semantic({ | |
| .pass_ordinal = pass_ordinal, | |
| .member_name = member.name, | |
| .classification = classification, | |
| .value = value, | |
| .offset = static_cast<uint32_t>(member.offset), | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/chain_executor.cpp` around lines 231 - 243, The current
logic freezes semantic records on first sight by using
resources.m_recorded_semantic_keys (semantic_record_key(pass_ordinal,
member.name)) to gate session.record_semantic, which causes stale live-derived
values (e.g., semantic_value fields like FrameCount and alias-backed *Size
members) to never update after the first frame/resize; instead, either move the
presence check to only suppress duplicate warnings (i.e., keep the
first-observation gate for warning emission but always call
session.record_semantic), or implement value-based change detection: store the
last recorded semantic_value per (pass_ordinal, member.name) and compare the
current semantic_value (including FrameCount and size fields) before calling
session.record_semantic only when it has changed; update
resources.m_recorded_semantic_keys usage accordingly and ensure any alias-backed
size fields are evaluated after bindings are prepared before
comparing/recording.
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <source_location> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== C++ standard declarations in CMake files =="
fd -HI 'CMakeLists\.txt$|\.cmake$' \
| xargs -r rg -n -C2 'CMAKE_CXX_STANDARD|cxx_std_20|target_compile_features'
echo
echo "== Usages of source_location and this header =="
rg -n -C2 'source_location|goggles/filter_chain/error\.hpp'Repository: goggles-dev/Goggles
Length of output: 8934
🏁 Script executed:
#!/bin/bash
echo "=== Library target providing the header ==="
rg -n -B5 'goggles/filter_chain/error\.hpp' src/render/CMakeLists.txt | head -20
echo
echo "=== Target that likely exports this header ==="
rg -n -B10 -A10 'add_library.*goggles_render|add_library.*goggles_filter_chain' src/render/CMakeLists.txt
echo
echo "=== Check for PUBLIC include directories and compile features ==="
rg -n -C3 'target_include_directories.*PUBLIC|target_compile_features' src/render/CMakeLists.txt | head -40
echo
echo "=== Package config files (for consumers) ==="
fd -HI '\.cmake$' cmake/ | xargs -r cat | rg -n -C3 'CXX_STANDARD|cxx_std_20'Repository: goggles-dev/Goggles
Length of output: 1697
🏁 Script executed:
#!/bin/bash
echo "=== Full src/render/CMakeLists.txt (goggles_render target section) ==="
sed -n '1,100p' src/render/CMakeLists.txt
echo
echo "=== Check for CMake package config or install targets ==="
rg -n 'install.*TARGET|export|GogglesConfig|goggles-config' --type cmake
echo
echo "=== Search for target_compile_features on goggles_render ==="
rg -n -C5 'target_compile_features.*goggles_render'Repository: goggles-dev/Goggles
Length of output: 5095
🏁 Script executed:
#!/bin/bash
echo "=== GogglesFilterChainConfig.cmake.in template ==="
cat cmake/GogglesFilterChainConfig.cmake.in
echo
echo "=== src/render/chain/CMakeLists.txt ==="
cat src/render/chain/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 3126
Add C++20 requirement to exported CMake config for package consumers.
The header uses std::source_location (lines 4, 29, 31-32) which requires C++20. While in-tree builds are protected by CMAKE_CXX_STANDARD 20 in CompilerConfig.cmake, the installed package config (GogglesFilterChainConfig.cmake.in) does not enforce C++20 for consumers.
Add INTERFACE_COMPILE_FEATURES "cxx_std_20" to both the GogglesFilterChain::goggles-filter-chain and goggles-filter-chain imported targets in the config template to ensure package consumers enforce C++20.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/include/goggles/filter_chain/error.hpp` at line 4, The
public header uses std::source_location (requires C++20) but the installed CMake
config template doesn't enforce C++20 for consumers; update the
GogglesFilterChainConfig.cmake.in template so that both the exported target
(GogglesFilterChain::goggles-filter-chain) and the imported legacy target
(goggles-filter-chain) get INTERFACE_COMPILE_FEATURES "cxx_std_20" added when
the targets are defined/installed, ensuring consumers compile with C++20
compatibility for symbols like std::source_location used in
src/render/chain/include/goggles/filter_chain/error.hpp.
| #include <string> | ||
| #include <utility> | ||
|
|
||
| namespace goggles { |
There was a problem hiding this comment.
Namespace does not match the filter_chain public boundary.
Line 8 declares namespace goggles, but this header lives under goggles/filter_chain/.... That mismatch can break call sites expecting goggles::filter_chain::Error/ErrorCode and weakens the provider seam normalization goal.
Suggested fix
-namespace goggles {
+namespace goggles::filter_chain {
@@
-} // namespace goggles
+} // namespace goggles::filter_chainAlso applies to: 68-68
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/include/goggles/filter_chain/error.hpp` at line 8, The
header declares types under namespace goggles but lives in goggles/filter_chain,
so update the namespace to goggles::filter_chain for the public types (Error,
ErrorCode, and any related declarations) to match the directory boundary; change
the opening namespace declaration from namespace goggles to namespace
goggles::filter_chain and update the matching closing comment/brace accordingly
so symbols are emitted as goggles::filter_chain::Error and
goggles::filter_chain::ErrorCode.
Comment on lines
+5
to
+9
| #include <cstdio> | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <nonstd/expected.hpp> | ||
| #include <utility> |
There was a problem hiding this comment.
Missing <source_location> header.
The factory functions make_error and make_result_ptr_error use std::source_location but the header <source_location> is not included. This will cause compilation failures on some compilers.
Proposed fix
`#include` <cstdio>
`#include` <cstdlib>
`#include` <memory>
`#include` <nonstd/expected.hpp>
+#include <source_location>
`#include` <utility>📝 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.
Suggested change
| #include <cstdio> | |
| #include <cstdlib> | |
| #include <memory> | |
| #include <nonstd/expected.hpp> | |
| #include <utility> | |
| `#include` <cstdio> | |
| `#include` <cstdlib> | |
| `#include` <memory> | |
| `#include` <nonstd/expected.hpp> | |
| `#include` <source_location> | |
| `#include` <utility> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/chain/include/goggles/filter_chain/result.hpp` around lines 5 - 9,
The file is missing the <source_location> header used by std::source_location in
the factory functions make_error and make_result_ptr_error; fix by adding an
`#include` <source_location> to the includes at the top of result.hpp so the
symbols used by make_error and make_result_ptr_error compile correctly with all
compilers.
Comment on lines
+25
to
28
| target_compile_definitions(goggles_render_shader_obj PRIVATE | ||
| VULKAN_HPP_NO_EXCEPTIONS | ||
| VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1 | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if public headers in this target include vulkan.hpp or vulkan_hpp variants
echo "=== Public headers in render/shader ==="
fd -e hpp -e h . src/render/shader
echo ""
echo "=== Check for vulkan.hpp includes in those headers ==="
rg -n '#include.*vulkan' src/render/shader --glob '*.hpp' --glob '*.h'
echo ""
echo "=== Also check chain/include headers since they're PUBLIC ==="
rg -n '#include.*vulkan' src/render/chain/include --glob '*.hpp' --glob '*.h' 2>/dev/null || echo "Path may not exist or no matches"Repository: goggles-dev/Goggles
Length of output: 503
🏁 Script executed:
cat -n src/render/shader/CMakeLists.txt | head -50Repository: goggles-dev/Goggles
Length of output: 1114
🏁 Script executed:
rg -n 'public_header|install|slang_reflect' src/render/shader/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 86
🏁 Script executed:
rg -n 'PUBLIC' src/render/shader/CMakeLists.txt | head -20Repository: goggles-dev/Goggles
Length of output: 124
Change compile definitions from PRIVATE to PUBLIC to prevent ODR violations.
slang_reflect.hpp is publicly exposed (via target_include_directories(PUBLIC)) and directly includes <vulkan/vulkan.hpp>. Consumers building against this library will include this header without the VULKAN_HPP_NO_EXCEPTIONS and VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1 definitions, causing struct layout and dispatch ABI mismatches. These definitions must be PUBLIC.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/render/shader/CMakeLists.txt` around lines 25 - 28, The compile
definitions for the target are currently applied as PRIVATE which causes
downstream consumers including slang_reflect.hpp (exposed via
target_include_directories(PUBLIC)) to be built without VULKAN_HPP_NO_EXCEPTIONS
and VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1, risking ODR/ABI mismatches; change the
call to target_compile_definitions(goggles_render_shader_obj ...) so the
definitions VULKAN_HPP_NO_EXCEPTIONS and VULKAN_HPP_DISPATCH_LOADER_DYNAMIC=1
are PUBLIC (not PRIVATE) so that consumers including <vulkan/vulkan.hpp> get the
same macro settings as the library.
| endif() | ||
|
|
||
| target_include_directories(goggles_util PUBLIC | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what headers from render/chain/include are actually used by util sources
echo "=== Headers in render/chain/include ==="
fd . src/render/chain/include --type f 2>/dev/null || echo "Directory not found"
echo ""
echo "=== Includes from util sources referencing chain headers ==="
rg -n '#include.*goggles/filter_chain' src/util/ 2>/dev/null || echo "No direct includes found"
echo ""
echo "=== Check if render/chain depends on util (potential circular dependency) ==="
rg -l 'goggles_util' src/render/chain/CMakeLists.txt 2>/dev/null || echo "No dependency found in chain CMakeLists"Repository: goggles-dev/Goggles
Length of output: 1388
🏁 Script executed:
#!/bin/bash
# Check what symbols from render/chain are actually used in util
echo "=== Content of util/error.hpp to see result.hpp usage ==="
cat -n src/util/error.hpp | head -30
echo ""
echo "=== Content of util/config.hpp to see scale_mode.hpp usage ==="
cat -n src/util/config.hpp | head -30
echo ""
echo "=== render/chain CMakeLists.txt to confirm util dependency ==="
rg -n 'goggles_util' src/render/chain/CMakeLists.txt -A 2 -B 2Repository: goggles-dev/Goggles
Length of output: 1356
🏁 Script executed:
#!/bin/bash
# Get full content of util headers and CMakeLists.txt context
echo "=== Full util/error.hpp ==="
cat -n src/util/error.hpp
echo ""
echo "=== render/chain/include/goggles/filter_chain/result.hpp ==="
head -40 src/render/chain/include/goggles/filter_chain/result.hpp
echo ""
echo "=== render/chain/include/goggles/filter_chain/scale_mode.hpp ==="
cat -n src/render/chain/include/goggles/filter_chain/scale_mode.hpp
echo ""
echo "=== CMakeLists.txt context around line 36 ==="
sed -n '30,45p' src/util/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 2701
🏁 Script executed:
#!/bin/bash
# Check if the includes are actually used (dead imports?) and verify full circular dependency
echo "=== Check where Error type is defined ==="
rg 'class Error\|struct Error' --type-list
echo ""
echo "=== Check if util code actually uses Result<T> from render/chain ==="
grep -n 'Result<' src/util/error.hpp src/util/config.hpp || echo "No Result<T> usage found"
echo ""
echo "=== Check render/chain/error.hpp ==="
head -30 src/render/chain/include/goggles/filter_chain/error.hpp
echo ""
echo "=== Verify full path: does util/config.hpp actually use ScaleMode? ==="
grep -n 'ScaleMode' src/util/config.hpp || echo "No ScaleMode usage found"Repository: goggles-dev/Goggles
Length of output: 1375
🏁 Script executed:
#!/bin/bash
# Verify the exact CMakeLists.txt line 36 that's mentioned in the review
echo "=== Full util/CMakeLists.txt to see target structure ==="
cat -n src/util/CMakeLists.txt
echo ""
echo "=== Check if Error/ErrorCode are defined in util or render/chain ==="
rg 'enum class ErrorCode\|struct Error' src/util/ src/render/chain/include/Repository: goggles-dev/Goggles
Length of output: 1986
🏁 Script executed:
#!/bin/bash
# Check if Error/ErrorCode are defined elsewhere or only in render/chain
echo "=== Check util directory structure ==="
find src/util -type f -name "*.hpp" -o -name "*.cpp" | sort
echo ""
echo "=== Search for Error/ErrorCode definitions in entire codebase ==="
rg 'enum class ErrorCode' --type-list 2>/dev/null
rg 'enum class ErrorCode'
rg 'struct Error \{'Repository: goggles-dev/Goggles
Length of output: 1582
Resolve circular dependency: util module should not depend on render/chain headers.
Adding ${CMAKE_SOURCE_DIR}/src/render/chain/include to util creates a circular dependency—util includes Error/ErrorCode and Result types from render/chain, while render/chain links against goggles_util. This inverts standard layering where render components depend on utilities, not vice versa.
The root issue is that Error and ErrorCode are defined in render/chain but are fundamental infrastructure types needed by util (see util/config.hpp using Result). Extract Error, ErrorCode, and Result to a dedicated utility types module (or move them to util directly) that both util and render/chain can depend on without circularity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/CMakeLists.txt` at line 36, The util CMakeLists should not include
render/chain headers because Error, ErrorCode, and Result<T> are causing a
circular dependency; extract those types into a shared utility module (e.g.,
goggles_types or move them directly into util) and update CMake and includes
accordingly: create a new target/library that defines Error, ErrorCode, and
Result<T> (or move their definitions into util headers referenced by
util/config.hpp), change render/chain and util to depend on that new target
instead of render/chain, and remove
`${CMAKE_SOURCE_DIR}/src/render/chain/include` from util's include list so the
circular link is eliminated.
| @@ -1,5 +1,6 @@ | |||
| #pragma once | |||
|
|
|||
| #include "../render/chain/include/goggles/filter_chain/scale_mode.hpp" | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify there are no remaining source-tree-relative includes to filter_chain public headers.
rg -nP '#include\s+"(\.\./)+render/chain/include/goggles/filter_chain/[^"]+"' src tests
# Expected: no matchesRepository: goggles-dev/Goggles
Length of output: 243
Use the public filter-chain include path, not a repo-relative path.
Line 3 hardcodes a source-tree layout and can fail in package/provider modes. This should consume the stable public header contract instead. The same issue exists in src/util/error.hpp:3.
Suggested fix
-#include "../render/chain/include/goggles/filter_chain/scale_mode.hpp"
+#include <goggles/filter_chain/scale_mode.hpp>📝 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.
Suggested change
| #include "../render/chain/include/goggles/filter_chain/scale_mode.hpp" | |
| `#include` <goggles/filter_chain/scale_mode.hpp> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/config.hpp` at line 3, The include in src/util/config.hpp currently
uses a repo-relative path to scale_mode.hpp
("../render/chain/include/goggles/filter_chain/scale_mode.hpp"); change it to
the public filter-chain include (e.g. use the stable public header path such as
"goggles/filter_chain/scale_mode.hpp" or "filter_chain/scale_mode.hpp" depending
on the project's public include root) so the header is resolved via the
installed/public include contract; make the equivalent change in
src/util/error.hpp where the same repo-relative include appears so both files
consume the public filter-chain header instead of hardcoded source-tree paths.
Comment on lines
+144
to
+146
| if(TARGET goggles_filter_chain_contract_tests) | ||
| add_test(NAME goggles_filter_chain_contract_tests COMMAND goggles_filter_chain_contract_tests) | ||
| endif() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the CMakeLists.txt file
find . -path "*/tests/CMakeLists.txt" -type fRepository: goggles-dev/Goggles
Length of output: 85
🏁 Script executed:
# Get the file size first
wc -l tests/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 87
🏁 Script executed:
# Read the relevant sections of the file
# Lines 144-146 and surrounding context
sed -n '130,160p' tests/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 1159
🏁 Script executed:
# Read lines 177-185 and context
sed -n '165,200p' tests/CMakeLists.txtRepository: goggles-dev/Goggles
Length of output: 1794
Double registration and missing properties on discovered contract tests.
When Catch.cmake is available, goggles_filter_chain_contract_tests is registered both via add_test() (line 144) and via catch_discover_tests() (line 196), causing the suite to run as both an aggregate executable and as individually discovered test cases. The ASAN and label properties set on the aggregate test (lines 177, 185) do not apply to the discovered individual cases. Either skip add_test() when discovery succeeds, or pass properties to catch_discover_tests() using the PROPERTIES parameter.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/CMakeLists.txt` around lines 144 - 146, The test target
goggles_filter_chain_contract_tests is being registered twice (via add_test and
via catch_discover_tests), causing duplicate runs and lost ASAN/label
properties; fix by removing or guarding the add_test registration when
Catch.cmake discovery is used (i.e., only call add_test if catch_discover_tests
is not available) OR remove add_test and instead pass the ASAN and label
properties to catch_discover_tests using its PROPERTIES argument so the
discovered individual cases inherit the same properties (apply to the
goggles_filter_chain_contract_tests target and the existing ASAN/label property
names used elsewhere).
Comment on lines
+90
to
+99
| if (features13.dynamicRendering != VK_TRUE) { | ||
| return; | ||
| } | ||
| features13.dynamicRendering = VK_TRUE; | ||
|
|
||
| VkDeviceCreateInfo device_info{}; | ||
| device_info.sType = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO; | ||
| device_info.queueCreateInfoCount = 1u; | ||
| device_info.pQueueCreateInfos = &queue_info; | ||
| device_info.pNext = &features13; |
There was a problem hiding this comment.
Vulkan feature chain incorrectly constructed.
Two issues here:
- Line 93 redundantly sets
dynamicRendering = VK_TRUEafter already verifying it's true on line 90. - Line 99 passes
&features13directly topNext, butfeatures2contains the baseVkPhysicalDeviceFeaturesthat should be included in the chain for complete feature enablement.
🔧 Proposed fix
vkGetPhysicalDeviceFeatures2(m_physical_device, &features2);
if (features13.dynamicRendering != VK_TRUE) {
return;
}
- features13.dynamicRendering = VK_TRUE;
VkDeviceCreateInfo device_info{};
device_info.sType = VK_STRUCTURE_TYPE_DEVICE_CREATE_INFO;
device_info.queueCreateInfoCount = 1u;
device_info.pQueueCreateInfos = &queue_info;
- device_info.pNext = &features13;
+ device_info.pNext = &features2;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/render/test_filter_chain_retarget_contract.cpp` around lines 90 - 99,
Remove the redundant assignment of features13.dynamicRendering after the feature
check and correctly build the pNext chain so the base features struct is
included: set features2.pNext to &features13 and then assign device_info.pNext =
&features2 (instead of &features13) so the chain begins with features2 and
continues to features13; keep the existing check that returns if
features13.dynamicRendering != VK_TRUE but do not reassign it.
There was a problem hiding this comment.
Pull request overview
This PR normalizes how Goggles consumes the goggles-filter-chain library across provider modes (in-tree/subdir/package), while moving “public support” types (Result/Error, controls, scale mode, Vulkan context, diagnostics entrypoints) onto the boundary-owned include surface and expanding contract coverage around retarget vs reload semantics.
Changes:
- Introduces a CMake provider-resolution module to always expose a normalized
goggles-filter-chaintarget, plus rehearsal presets/tasks for package consumption. - Moves/forwards previously Goggles-internal support headers to
src/render/chain/include/goggles/filter_chain/*and updates host/backend includes accordingly. - Splits test registration into host integration vs boundary contract executables and adds a Vulkan-backed retarget contract test.
Reviewed changes
Copilot reviewed 73 out of 73 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/render/test_vulkan_backend_subsystem_contracts.cpp | Updates include-path assertions to require the canonical boundary header. |
| tests/render/test_filter_chain_retarget_contract.cpp | Adds Vulkan-backed contract test for retarget preserving runtime state. |
| tests/render/test_filter_boundary_contracts.cpp | Updates boundary include audits and tightens host/backend access constraints. |
| tests/CMakeLists.txt | Splits host vs contract test targets and adds provider-aware wiring. |
| src/util/error.hpp | Converts util error header into a forwarder to boundary result.hpp. |
| src/util/config.hpp | Moves ScaleMode to boundary-owned header and updates include. |
| src/util/CMakeLists.txt | Splits util logging into an object library and adds boundary include dirs. |
| src/render/texture/CMakeLists.txt | Adds boundary include dir; adjusts link/defs visibility. |
| src/render/shader/shader_runtime.hpp | Switches to boundary-owned diagnostics + result headers. |
| src/render/shader/retroarch_preprocessor.hpp | Switches to boundary-owned diagnostics + result headers. |
| src/render/shader/CMakeLists.txt | Adds boundary include dir; adjusts link/defs visibility. |
| src/render/chain/vulkan_result.hpp | Switches from util/error.hpp to boundary result.hpp. |
| src/render/chain/vulkan_context.hpp | Makes legacy header a forwarder to boundary vulkan context. |
| src/render/chain/preset_parser.hpp | Switches to boundary result.hpp. |
| src/render/chain/pass.hpp | Switches to boundary scale_mode.hpp and removes util includes. |
| src/render/chain/include/goggles/filter_chain/vulkan_context.hpp | Adds boundary-owned VulkanContext definition. |
| src/render/chain/include/goggles/filter_chain/scale_mode.hpp | Adds boundary-owned ScaleMode + to_string. |
| src/render/chain/include/goggles/filter_chain/result.hpp | Adds boundary-owned Result/Error helpers and TRY/MUST/ASSERT macros. |
| src/render/chain/include/goggles/filter_chain/filter_controls.hpp | Adds boundary-owned filter control descriptors and helpers. |
| src/render/chain/include/goggles/filter_chain/error.hpp | Adds boundary-owned Error/ErrorCode + naming. |
| src/render/chain/include/goggles/filter_chain/diagnostics/source_provenance.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/log_sink.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/gpu_timestamp_pool.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_sink.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_session.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_policy.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_event.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/include/goggles/filter_chain/diagnostics/compile_report.hpp | Adds diagnostics forwarder header in boundary surface. |
| src/render/chain/framebuffer.hpp | Switches to boundary result.hpp. |
| src/render/chain/filter_pass.hpp | Switches diagnostics include to boundary surface. |
| src/render/chain/filter_controls.hpp | Makes legacy header a forwarder to boundary filter controls. |
| src/render/chain/chain_runtime.hpp | Switches to boundary diagnostics/result/scale_mode headers. |
| src/render/chain/chain_runtime.cpp | Switches diagnostics include to boundary surface. |
| src/render/chain/chain_resources.hpp | Adds state to dedupe recorded diagnostic entries. |
| src/render/chain/chain_resources.cpp | Clears new diagnostic-recording state during install/shutdown. |
| src/render/chain/chain_executor.hpp | Switches to boundary diagnostics headers. |
| src/render/chain/chain_executor.cpp | Dedupe semantic/binding diagnostics emission based on observed state. |
| src/render/chain/chain_builder.hpp | Switches to boundary diagnostics headers. |
| src/render/chain/chain_builder.cpp | Switches to boundary diagnostics headers. |
| src/render/chain/api/cpp/goggles_filter_chain.hpp | Switches to boundary result.hpp and public C header include. |
| src/render/chain/api/c/goggles_filter_chain.h | Documents retarget vs reload and explicit host/boundary ownership notes. |
| src/render/chain/api/c/goggles_filter_chain.cpp | Switches to boundary diagnostics/result/scale_mode headers. |
| src/render/chain/CMakeLists.txt | Adds boundary include dir; adjusts link/defs visibility. |
| src/render/backend/vulkan_error.hpp | Switches to boundary result.hpp. |
| src/render/backend/vulkan_debug.hpp | Switches to boundary result.hpp. |
| src/render/backend/vulkan_context.hpp | Switches VulkanContext include to boundary surface. |
| src/render/backend/vulkan_backend.hpp | Switches controls/result/scale_mode to boundary surface. |
| src/render/backend/vulkan_backend.cpp | Routes recording through FilterChainController::record() seam. |
| src/render/backend/render_output.hpp | Switches to boundary result.hpp. |
| src/render/backend/filter_chain_controller.hpp | Switches to boundary headers and adds record() API. |
| src/render/backend/filter_chain_controller.cpp | Implements record() with initialization guard. |
| src/render/backend/external_frame_importer.hpp | Switches to boundary result.hpp. |
| src/render/backend/CMakeLists.txt | Adds chain C API include dir for backend build. |
| src/render/CMakeLists.txt | Integrates provider resolution; installs boundary headers + package config when owned. |
| scripts/task/rehearse-filter-chain-provider.sh | Adds provider rehearsal workflow for in-tree + package consumer modes. |
| scripts/task/help.sh | Documents the new rehearsal task. |
| pixi.toml | Registers rehearse-filter-chain-provider Pixi task. |
| openspec/specs/render-pipeline/spec.md | Updates retarget-vs-reload requirements and async lifecycle contract text. |
| openspec/specs/goggles-filter-chain/spec.md | Updates boundary ownership and normalized provider contract requirements. |
| openspec/specs/filter-chain-cpp-wrapper/spec.md | Defines wrapper retarget contract + extraction-safe public surface. |
| openspec/specs/filter-chain-c-api/spec.md | Defines post-retarget output contract and public-surface isolation. |
| openspec/specs/build-system/spec.md | Defines normalized provider selection and rehearsal workflow requirements. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/tasks.md | Archives milestone tasks checklist for this boundary cleanup. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/render-pipeline/spec.md | Captures archived delta for render-pipeline spec. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/goggles-filter-chain/spec.md | Captures archived delta for goggles-filter-chain spec. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/filter-chain-cpp-wrapper/spec.md | Captures archived delta for C++ wrapper spec. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/filter-chain-c-api/spec.md | Captures archived delta for C ABI spec. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/specs/build-system/spec.md | Captures archived delta for build-system spec. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/proposal.md | Archives proposal reframing as consumer-boundary cleanup milestone. |
| openspec/changes/archive/2026-03-13-normalize-filter-chain-consumer-boundary/design.md | Archives design describing provider seam, boundary headers, and test split. |
| cmake/GogglesFilterChainProvider.cmake | Adds provider-resolution + target normalization for in-tree/subdir/package. |
| cmake/GogglesFilterChainConfig.cmake.in | Adds install-package config for the filter-chain boundary library. |
| CMakePresets.json | Adds provider rehearsal configure/build/test presets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
1
to
+3
| #pragma once | ||
|
|
||
| #include <cstdint> | ||
| #include <cstdio> | ||
| #include <cstdlib> | ||
| #include <memory> | ||
| #include <nonstd/expected.hpp> | ||
| #include <source_location> | ||
| #include <string> | ||
| #include <utility> | ||
|
|
||
| namespace goggles { | ||
|
|
||
| /// @brief Error codes used by `goggles::Error`. | ||
| enum class ErrorCode : std::uint8_t { | ||
| ok, | ||
| file_not_found, | ||
| file_read_failed, | ||
| file_write_failed, | ||
| parse_error, | ||
| invalid_config, | ||
| vulkan_init_failed, | ||
| vulkan_device_lost, | ||
| shader_compile_failed, | ||
| shader_load_failed, | ||
| input_init_failed, | ||
| invalid_data, | ||
| unknown_error | ||
| }; | ||
|
|
||
| /// @brief Structured error for `Result<T>` operations. | ||
| struct Error { | ||
| ErrorCode code; | ||
| std::string message; | ||
| std::source_location location; | ||
|
|
||
| Error(ErrorCode error_code, std::string msg, | ||
| std::source_location loc = std::source_location::current()) | ||
| : code(error_code), message(std::move(msg)), location(loc) {} | ||
| }; | ||
|
|
||
| /// @brief Project-wide fallible operation return type. | ||
| template <typename T> | ||
| using Result = nonstd::expected<T, Error>; | ||
|
|
||
| /// @brief Convenience alias for `Result<std::unique_ptr<T>>`. | ||
| template <typename T> | ||
| using ResultPtr = Result<std::unique_ptr<T>>; | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto make_error(ErrorCode code, std::string message, | ||
| std::source_location loc = std::source_location::current()) | ||
| -> Result<T> { | ||
| return nonstd::make_unexpected(Error{code, std::move(message), loc}); | ||
| } | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto make_result_ptr(std::unique_ptr<T> ptr) -> ResultPtr<T> { | ||
| return ResultPtr<T>{std::move(ptr)}; | ||
| } | ||
|
|
||
| template <typename T> | ||
| [[nodiscard]] inline auto | ||
| make_result_ptr_error(ErrorCode code, std::string message, | ||
| std::source_location loc = std::source_location::current()) -> ResultPtr<T> { | ||
| return nonstd::make_unexpected(Error{code, std::move(message), loc}); | ||
| } | ||
|
|
||
| /// @brief Returns a stable string name for an `ErrorCode` value. | ||
| [[nodiscard]] constexpr auto error_code_name(ErrorCode code) -> const char* { | ||
| switch (code) { | ||
| case ErrorCode::ok: | ||
| return "ok"; | ||
| case ErrorCode::file_not_found: | ||
| return "file_not_found"; | ||
| case ErrorCode::file_read_failed: | ||
| return "file_read_failed"; | ||
| case ErrorCode::file_write_failed: | ||
| return "file_write_failed"; | ||
| case ErrorCode::parse_error: | ||
| return "parse_error"; | ||
| case ErrorCode::invalid_config: | ||
| return "invalid_config"; | ||
| case ErrorCode::vulkan_init_failed: | ||
| return "vulkan_init_failed"; | ||
| case ErrorCode::vulkan_device_lost: | ||
| return "vulkan_device_lost"; | ||
| case ErrorCode::shader_compile_failed: | ||
| return "shader_compile_failed"; | ||
| case ErrorCode::shader_load_failed: | ||
| return "shader_load_failed"; | ||
| case ErrorCode::input_init_failed: | ||
| return "input_init_failed"; | ||
| case ErrorCode::invalid_data: | ||
| return "invalid_data"; | ||
| case ErrorCode::unknown_error: | ||
| return "unknown_error"; | ||
| } | ||
| return "unknown"; | ||
| } | ||
|
|
||
| } // namespace goggles | ||
|
|
||
| // NOLINTBEGIN(cppcoreguidelines-macro-usage) | ||
|
|
||
| /// @brief Propagates an error or returns the contained value (expression-style). | ||
| /// | ||
| /// Similar to Rust's `?` operator. The expression must yield a `Result<T>`. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_TRY(expr) \ | ||
| ({ \ | ||
| auto _try_result = (expr); \ | ||
| if (!_try_result) \ | ||
| return nonstd::make_unexpected(_try_result.error()); \ | ||
| std::move(_try_result).value(); \ | ||
| }) | ||
|
|
||
| /// @brief Aborts on error or returns the contained value (expression-style). | ||
| /// | ||
| /// Use for internal invariants where failure indicates a bug. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_MUST(expr) \ | ||
| ({ \ | ||
| auto _must_result = (expr); \ | ||
| if (!_must_result) { \ | ||
| auto& _err = _must_result.error(); \ | ||
| std::fprintf(stderr, "GOGGLES_MUST failed at %s:%u in %s\n %s: %s\n", \ | ||
| _err.location.file_name(), _err.location.line(), \ | ||
| _err.location.function_name(), goggles::error_code_name(_err.code), \ | ||
| _err.message.c_str()); \ | ||
| std::abort(); \ | ||
| } \ | ||
| std::move(_must_result).value(); \ | ||
| }) | ||
|
|
||
| /// @brief Aborts when an invariant is violated. | ||
| // NOLINTNEXTLINE(bugprone-macro-parentheses) | ||
| #define GOGGLES_ASSERT(condition, ...) \ | ||
| do { \ | ||
| if (!(condition)) { \ | ||
| std::fprintf(stderr, "GOGGLES_ASSERT failed: %s at %s:%u in %s\n", #condition, \ | ||
| __FILE__, __LINE__, __func__); \ | ||
| __VA_OPT__(do { \ | ||
| std::fprintf(stderr, " "); \ | ||
| std::fprintf(stderr, __VA_ARGS__); \ | ||
| std::fprintf(stderr, "\n"); \ | ||
| } while (false);) \ | ||
| std::abort(); \ | ||
| } \ | ||
| } while (false) | ||
|
|
||
| // NOLINTEND(cppcoreguidelines-macro-usage) | ||
| #include "../render/chain/include/goggles/filter_chain/result.hpp" |
Comment on lines
+1
to
+3
| #pragma once | ||
|
|
||
| #include <util/diagnostics/compile_report.hpp> |
Comment on lines
+127
to
+134
| target_link_libraries(goggles-filter-chain | ||
| PUBLIC | ||
| Vulkan::Vulkan | ||
| PRIVATE | ||
| spdlog::spdlog | ||
| slang::slang | ||
| stb_image | ||
| ) |
Comment on lines
+66
to
+76
| install(FILES | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/compile_report.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_event.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_policy.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_session.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/diagnostic_sink.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/gpu_timestamp_pool.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/log_sink.hpp | ||
| ${CMAKE_SOURCE_DIR}/src/render/chain/include/goggles/filter_chain/diagnostics/source_provenance.hpp | ||
| DESTINATION include/goggles/filter_chain/diagnostics | ||
| ) |
Comment on lines
+11
to
+20
| set(GOGGLES_FILTER_CHAIN_CONTRACT_TEST_SOURCES | ||
| render/test_shader_runtime.cpp | ||
| render/test_retroarch_preprocessor.cpp | ||
| render/test_preset_parser.cpp | ||
| render/test_slang_reflect.cpp | ||
| render/test_filter_chain.cpp | ||
| render/test_filter_controls.cpp | ||
| render/test_filter_chain_c_api_contracts.cpp | ||
| render/test_filter_chain_retarget_contract.cpp | ||
| ) |
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
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 by CodeRabbit
New Features
Documentation