fix: frame geometry and flush paint events before screenshot capture - #102
fix: frame geometry and flush paint events before screenshot capture#102Merlz wants to merge 1 commit into
Conversation
`saveImage` could run before the renderer had drawn, and against a viewport that did not contain the model, so it wrote a perfectly valid PNG with nothing in it. Reproduced on FreeCAD 1.1.3: capturing a visible 20mm box gave a 3410-byte image containing only background; with this change, a correct 6364-byte render. Fixed by flushing pending paint events and fitting the view before the grab. Confirmed separately that `fitAll()` is what matters — the geometry was framed out of view, not merely un-drawn — so no camera *orientation* call is added; `view_angle` continues to own that. The camera is saved and restored around the capture. Without that, every screenshot re-frames the view, which has two consequences: `zoom_in`, `zoom_out` and `set_camera_position` become unobservable, since a screenshot is their only feedback, and `get_screenshot` — documented read-only — permanently moves the user's 3D view. Verified on 1.1.3 that getCamera/setCamera round-trips byte-identically across repeated captures. Applied to all three bridges. Adds tests asserting on the generated source, since the existing get_screenshot tests mock at the bridge boundary and cannot see this code; the ordering assertions fail if the restore moves before the grab. Fixes spkane#92
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
… fix Our fixes: - .env in the working directory no longer crashes startup (spkane#105) - PartDesign features that silently do nothing are now caught (spkane#99) - empty-body Shape access no longer crashes pad onto a fresh body - mcp pinned below 2.x so a fresh pip install imports Backported from unmerged upstream PRs, original authorship preserved: - spkane#75 type() instead of __class__ for FreeCAD C++ bindings - spkane#101 count external geometry by subelement - spkane#102 flush paint events before screenshot capture Verified end to end against FreeCAD 1.1.3 in embedded mode: MCP tool -> EmbeddedBridge -> FreeCAD, 152 tools served over stdio. Claude-Session: https://claude.ai/code/session_01Y72E9EtDrSDkA9n1Fnu21r
1 participant
Fixes #92.
Summary
Confirmed reproducible on FreeCAD 1.1.3, and there were two distinct causes rather than one.
saveImagecould run before the renderer had drawn, and against a viewport that did not contain the model. Either produces a perfectly valid PNG with nothing in it, which is why the symptom reads as "screenshots are generated but objects are absent".Reproduction, driving the bridge directly with a visible 20mm box in the active document:
Approach
Before the grab: flush pending paint events, then fit the view.
I checked which of the two causes was actually doing the work rather than assuming, because it changes the fix. Zooming far out so the geometry left the frustum and then applying
updateGui()+fitAll()recovered a correct capture — so framing was the dominant factor, not just the paint race. That is whyfitAll()is here and why no camera orientation call is added:get_screenshot's ownview_angleparameter owns orientation, and forcing e.g. axonometric would override what the caller asked for.After the grab: restore the camera.
That second half matters more than it looks. Re-framing on every capture has two side effects:
zoom_in,zoom_outandset_camera_positionchange only the camera, and a screenshot is their only feedback channel. If every screenshot re-fits, those three tools become unobservable — effectively inert from a caller's point of view.get_screenshotis documented read-only (it is listed under "Tools NOT Requiring Transactions"), yet it would permanently move the user's 3D view on every call.Saving and restoring around the capture keeps the image correctly framed without persisting the change. Verified on 1.1.3 that
getCamera()/setCamera()round-trips byte-identically across repeated captures, including the position and orientation fields, and that the camera ends where it started after two consecutive screenshots from a deliberately zoomed-in view.Applied to all three bridges (
xmlrpc,socket,embedded) for consistency, though I could only exercisexmlrpclive.Verification
main+ 3 added).ruff check/ruff format --checkcleanTest Plan
uv run pytest tests/unit— 423 passedruff check src/ tests/cleanruff format --check src/ tests/cleansocketandembeddedbridges exercised live — patched identically and covered by the new codegen tests, but I only have xmlrpc runningNotes on the tests
New file
tests/unit/test_bridge_screenshot_codegen.py, parametrized across all three bridges.The existing
test_get_screenshot_*tests mock at the bridge boundary, so they cannot see this code — it is generated inside the bridges. That is a large part of why the bug was invisible to CI. These tests mockexecute_python, capture the generated source, and assert:updateGui,fitAllandgetCameraall precedesaveImagesetCamerafollowssaveImage— the regression guard for the collateral damage described aboveI confirmed the ordering assertions are not vacuous by temporarily moving the restore before the grab: the test fails on
assert code.index("setCamera") > code.index("saveImage").