This repository was archived by the owner on Aug 12, 2026. It is now read-only.
feat(viewer): restore reproducible archival rendering - #151
Merged
Merged
Conversation
- add an opt-in macOS CGAL/Qt renderer driven by versioned manifests - publish canonical fixture provenance and the reproducible hero workflow - keep default builds headless while adding viewer build and check recipes Closes #98
WalkthroughThis PR restores an opt-in macOS CGAL/Qt viewer. It adds versioned fixture and manifest contracts, strict artifact validation, offscreen rendering, smoke tests, build workflows, editor configurations, and documentation while keeping headless builds unchanged. ChangesArchival viewer workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Developer
participant Justfile
participant ArtifactValidator
participant CDTViewer
participant CGALQt
participant CanonicalPNG
Developer->>Justfile: run viewer-render
Justfile->>CDTViewer: render hero manifest
CDTViewer->>ArtifactValidator: validate manifest and fixture contract
ArtifactValidator-->>CDTViewer: return validated fixture path
CDTViewer->>CGALQt: load triangulation and create scene
CGALQt->>CanonicalPNG: write PNG output
CDTViewer->>ArtifactValidator: validate rendered image
ArtifactValidator-->>Justfile: return validation status
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
acgetchell
enabled auto-merge
August 8, 2026 04:59
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.zed/tasks.json:
- Around line 1-37: Add a dedicated Zed task alongside the existing CDT++ tasks
for the viewer workflow, invoking the existing just viewer-render or
viewer-build recipe. Preserve the current task structure and settings, including
the worktree root as cwd and saving all output.
In `@Justfile`:
- Around line 411-414: Update the viewer-build recipe’s OS dispatch to use os()
== "macos" for the _build-viewer-unix branch, while retaining explicit
unsupported-platform errors for both Windows and Linux.
In `@scripts/tests/test_validate_viewer_artifacts.py`:
- Around line 25-52: The test suite lacks coverage for the structural-only
validation mode and image failure paths. Extend
test_repository_viewer_artifacts_validate with a copied PNG mutation case that
succeeds under validate(manifest, canonical=False) but fails under canonical
validation, plus cases asserting wrong dimensions and undersized image files are
rejected with the validator’s stable error.
In `@scripts/validate_viewer_artifacts.py`:
- Around line 254-258: Update the validation error handling around validate in
the CLI entry point to catch UnicodeDecodeError alongside the existing file,
artifact, and JSON/schema errors. Preserve the stable “Viewer artifact
validation failed” message and exit code for invalid UTF-8 manifests or
sidecars.
- Around line 177-189: Extend the provenance validation in the loop using
provenance_fields to compare fixture.provenance.topology with
metadata["topology"], not desired.topology. Add initial_radius and
foliation_spacing checks against their sidecar keys, parsing both values
numerically and applying an explicit precision policy before raising
ViewerArtifactError on mismatch.
In `@src/cdt-viewer.cpp`:
- Around line 793-796: Replace the hardcoded point-match tolerance in the
is_point lambda with a Render_config value populated from a versioned manifest
field such as style.point_color_match_tolerance. Update manifest parsing and
schema handling consistently, preserving the existing alpha check and
color-distance comparison while ensuring the tolerance is part of the frozen
render configuration.
- Around line 383-396: Add validation in the render-manifest parsing flow after
the post-parse checks to reject minimum_foreground_pixels greater than width ×
height, using size_t arithmetic and the specified invalid-argument message. Also
bound oversampling before saveSnapshot can derive framebuffer dimensions,
preserving the existing lower bound while rejecting values that could request
impractical or non-representable allocations; anchor both checks to the existing
render validation logic and result.render fields.
- Around line 973-981: Preserve the exact CGAL and Qt version comparison in the
manifest validation around the renderer startup path to maintain archival
reproducibility. Pin the vcpkg baseline used by the viewer preset so toolchain
versions cannot change implicitly, and document this exact-version policy and
the required manifest/canonical-image regeneration process in
docs/reproducibility.md.
- Around line 757-813: Replace per-pixel QImage::pixelColor/setPixelColor calls
in outline_face_boundaries (src/cdt-viewer.cpp:757-813) with constBits/bits
scanline access, QRgb values, and a QRgb-based color-distance helper while
preserving boundary and point-outline behavior. Also update foreground_pixels
(src/cdt-viewer.cpp:821-840) to use the same scanline access and direct
packed-QRgb comparison; both sites require changes.
- Around line 626-648: Add a concise one-line comment immediately above
orient_outward documenting that its radial normal test assumes the mesh is
star-shaped about the origin, as relied on by the spherical fixture. Do not
change the winding logic.
- Around line 227-248: Add a concise explanatory comment in require_integer
immediately above the MAX_EXACT_JSON_INTEGER comparison, documenting that the
2^53 clamp is required to reject values that may round past integer limits on
platforms with 64-bit long double and to keep overflow behavior
architecture-independent. Keep the existing validation logic unchanged.
- Around line 539-543: Update the topology mismatch exception in the fixture
validation logic to include the manifest’s expected vertices, edges, faces,
simplices, and timeslice range alongside the reported actual values. Preserve
the existing mismatch context while making the error self-contained for
regenerated fixtures.
- Around line 924-928: Add a single-shot watchdog around the headless execution
path before application.exec() that sets render_error to a clear timeout message
and calls application.exit(EXIT_FAILURE) if init() never completes. Ensure the
watchdog is cancelled or prevented from affecting successful runs, while
preserving the existing render_error check and return behavior.
- Around line 149-159: Add a member flag to the viewer and update init() so the
callback is scheduled only once, even if OpenGL reinitializes; preserve the
existing callback behavior and set the guard when scheduling it.
- Line 660: Extract the repeated edge-scope predicates into named helpers
adjacent to Render_config, including draws_scene_edges and
draws_screen_space_edges as needed. Update the options.ignore_all_edges call and
the corresponding logic in configure_viewer and outline_face_boundaries to reuse
these helpers, ensuring all edge-scope decisions remain consistent.
- Around line 888-916: Declare render_error before viewer so the callback’s
referenced state outlives viewer and its stored lambda. In the render callback
registered by viewer.after_initialization, retain the std::exception handler and
add a catch-all handler that records an appropriate diagnostic in render_error
and exits with EXIT_FAILURE, preventing non-standard exceptions from escaping
the Qt event handler.
In `@src/CMakeLists.txt`:
- Around line 51-61: Replace the generated viewer_integer_overflow_manifest
logic in the CMake configuration with a direct reference to a dedicated invalid
manifest fixture under viewer/manifests/v1/. Add the fixture containing an
integer-overflow vertices value, and use that path in the viewer failure test
instead of reading and string-replacing viewer_manifest.
- Around line 42-49: Document near the cdt-viewer-smoke add_test declaration
that this test requires the Cocoa platform and an active GUI session because
cdt-viewer uses QApplication and an OpenGL-backed CGAL viewer. Explicitly state
that QT_QPA_PLATFORM=offscreen is unsupported unless the renderer is redesigned
around QOffscreenSurface, and do not add it as the test environment.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: cf921020-12bc-4d32-9633-40fcad7fff3f
⛔ Files ignored due to path filters (1)
docs/images/S3-7-27528-I1-R1.pngis excluded by!**/*.png
📒 Files selected for processing (22)
.zed/debug.json.zed/tasks.jsonCMakeLists.txtCMakePresets.jsonJustfileREADME.mddocs/cgal-integration.mddocs/reproducibility.mddocs/viewer.mdpyproject.tomlscripts/build.shscripts/pkgx-build.shscripts/tests/test_justfile_discoverability.pyscripts/tests/test_validate_viewer_artifacts.pyscripts/validate_viewer_artifacts.pysrc/CMakeLists.txtsrc/cdt-viewer.cppvcpkg.jsonviewer/README.mdviewer/fixtures/v1/S3-7-27528-I1-R1-seed30.off.metaviewer/manifests/v1/hero.jsonviewer/schema/render-manifest-v1.schema.json
Commit the hash-pinned OFF payload required by clean checkouts and mark it binary so cross-platform line-ending conversion cannot invalidate its digest.
- validate exact manifest provenance, topology, and canonical PNG identity - bound rendering controls and make foreground matching manifest-driven - make one-shot headless rendering fail safely and reject unsupported platforms
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
viewer/schema/render-manifest-v1.schema.json (1)
395-397: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftEnforce
minimum_foreground_pixelsduring structural validation.Lines 395-397 require this value, but
_validate_imagedoes not inspect image pixels. In--structural-onlymode, a valid fully transparent PNG can pass when rendering produced no geometry. Count foreground pixels and reject output below this manifest limit. Add a transparent-image test for this path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@viewer/schema/render-manifest-v1.schema.json` around lines 395 - 397, Update _validate_image to count non-transparent foreground pixels and reject images whose count is below the manifest’s minimum_foreground_pixels value, including in --structural-only mode. Preserve existing validation behavior for images meeting the limit, and add a test covering a fully transparent PNG that must be rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@viewer/schema/render-manifest-v1.schema.json`:
- Around line 395-397: Update _validate_image to count non-transparent
foreground pixels and reject images whose count is below the manifest’s
minimum_foreground_pixels value, including in --structural-only mode. Preserve
existing validation behavior for images meeting the limit, and add a test
covering a fully transparent PNG that must be rejected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0246b1b2-1473-463f-b406-1d20902994e5
📒 Files selected for processing (13)
.gitattributes.gitignore.zed/tasks.jsonJustfiledocs/reproducibility.mdscripts/tests/test_validate_viewer_artifacts.pyscripts/validate_viewer_artifacts.pysrc/CMakeLists.txtsrc/cdt-viewer.cppviewer/fixtures/v1/S3-7-27528-I1-R1-seed30.offviewer/manifests/v1/hero.jsonviewer/manifests/v1/invalid-integer-overflow.jsonviewer/schema/render-manifest-v1.schema.json
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #98
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests