Skip to content

pycvc_gl: fix broken keepalive hooks, StageLighting setters, and build-tree package layout - #495

Merged
transfix merged 1 commit into
masterfrom
fix/pycvc-gl-tests
Sep 30, 2026
Merged

transfix merged 1 commit into
masterfrom
fix/pycvc-gl-tests

Conversation

@transfix

@transfix transfix commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Problem

Three pycvc ctest cases failed on master (verified at d6ac6a7 and d5eb34e, built with the cvcpkg swig 4.4.1). CI never sets CVC_BUILD_PYCVC, so nothing caught them. Two of them hid further bugs behind their first error.

Fixes

1. pycvc_ari: broken %pythonappend keepalive hooks, then a teardown use-after-free

  • Wrong assumption about proxy shape. The keepalive hooks read args[0], but SWIG only emits def f(self, *args) for overloaded functions. A single-signature function gets named parameters (def __init__(self, viewer)), where args doesn't exist, so the hook raises NameError on every call.
  • Five hooks were broken this way: the ImGuiOverlay constructor and attachCamera, AriRuntime, VolRenNode and VolSliceNode. None of those could be constructed or called from Python. They now name the parameter directly, as addSceneViewport already did, and the misleading "only the *args ctor wrappers get it" comment is corrected.
  • A crash hidden behind the NameError. Once construction worked, the test segfaulted at teardown. SceneRenderer borrows its SceneGraph without a keepalive, so when a function's frame released sg before view, the scene was freed first and ~SceneRenderer → close() → ~ViewportManager → SceneGraph::setRenderer touched freed memory (confirmed with a gdb backtrace). SceneRenderer now keeps its scene alive. An audit of every wrapped constructor that borrows a scene, renderer, camera, overlay or viewport found this was the only one missing a keepalive; Viewport can't be constructed from Python.

2. pycvc_gl_world: a real cvcGL bug in StageLighting

  • The rig's own change callback clobbered its setters. handleStateChanged re-reads every field from state. The multi-field setters wrote their fields one at a time with that callback live, so the first write pulled the other fields' old values back in, and only one field stuck. Measured:
    • setStage(1,2,3,10) → (1, 0, 0, <old radius>)
    • setWash(0.7,5,2.5) → only the intensity changed
    • apply_preset("dramatic") → only key_intensity changed
  • The fix. setStage, setKey and setWash, plus applyPreset's seedState(), now write to state inside state_init_scope, the same idiom CameraController::seedState and FpsHud use, and then call apply() explicitly. Bound UI still sees every write, and outside single-key edits still flow in (both tested).
  • Constructor unchanged on purpose. The constructor's seedState() is left alone because its callback is what builds the lights at construction.
  • Side effect: cvcgl_stage_caster_truth's wash-count sweep now actually varies the count; it silently didn't before. It still passes.

3. pycvc_gl_chase_parity: build-tree layout didn't match the installed package

pycvc_gl installs as a package: the proxy is __init__.py, the extension lives inside it, and pymod_gl/*.py are submodules. The tests ran against the flat build-tree pycvc_gl.py, so pycvc_gl.camera couldn't resolve. A new pycvc_gl_pkg target stages the install layout under build/bindings/pycvc/pkg/, and every pycvc_gl test now imports that. They now test exactly what users get.

Guards, so these stay fixed

  • pycvc_proxy_hooks (new): parses the generated pycvc and pycvc_gl proxies and fails if any function references an undefined name. It flagged exactly the 5 broken hooks, with no false positives. It also catches the reverse case: a named-parameter hook that breaks when someone adds an overload and the proxy flips to *args.
  • cvcgl_stage_setters (new C++ test, headless, runs in CI): every field of setStage, frameBounds, setKey, setWash and applyPreset must reach both the object and state, and outside state edits must still flow in. With the fix reverted it fails on exactly the dropped fields.
  • test_pycvc_gl_imgui: the uiScale round-trip now runs only on a live overlay, because an inert one (no CVC_ENABLE_IMGUI) is a documented no-op. The check never actually ran before: the NameError was caught and reported as a SKIP.

Verification

Built with the cvcpkg swig only: SWIG_EXECUTABLE, SWIG_LIB and SWIG_DIR all come from a cvcpkg prefix (swig 4.4.1). ctest -R '^(pycvc|cvcgl_)' passes 66/66. pycvc_ari really renders (RENDER_OK, "AriRuntime test: OK") rather than taking its no-GL skip. git clang-format origin/master is clean.

On top of current master (f9f669e, after #493): a local trial merge rebuilds cleanly, and ctest -R '^(pycvc|cvcgl_|Ariadne)' passes 356/356.

…ing setters, build-tree package)

All three failed on master (CI never builds pycvc, so nothing caught them):

- pycvc_ari: ImGuiOverlay(view) raised NameError. %pythonappend hooks read
  `args[0]`, but SWIG emits `def f(self, *args)` only for OVERLOADED
  functions; single-signature ones get named parameters, where `args` does
  not exist. Five hooks were broken this way -- ImGuiOverlay ctor +
  attachCamera, AriRuntime, VolRenNode, VolSliceNode -- so none of those
  could be constructed/called from Python. They now reference the parameter
  by name (as addSceneViewport already did).
  Getting past that exposed a teardown segfault: SceneRenderer borrowed its
  SceneGraph with no keepalive, so a frame dropping `sg` before `view` freed
  the scene first and ~SceneRenderer -> ~ViewportManager ->
  SceneGraph::setRenderer touched freed memory. SceneRenderer now keeps its
  scene alive (the only borrowing, Python-constructible ctor without one).
- pycvc_gl_world: a real cvcGL bug. StageLighting's own change callback
  re-reads EVERY field from state, and the multi-field setters wrote their
  fields one at a time with it live, so only the first stuck:
  setStage(1,2,3,10) -> (1,0,0,<old>), setWash kept only the intensity,
  applyPreset kept one field of the preset. setStage/setKey/setWash and
  applyPreset's seedState now mirror under state_init_scope (the idiom
  CameraController/FpsHud use) and apply() explicitly; bound UI still sees
  every write, and outside single-key edits still flow in.
- pycvc_gl_chase_parity: `pycvc_gl.camera` is a submodule of the INSTALLED
  package, but tests ran against the flat build-tree pycvc_gl.py. A
  pycvc_gl_pkg target now stages the install layout (proxy as __init__.py,
  extension, pymod_gl/*.py) under pkg/, and the pycvc_gl tests import it.

Guards, so these stay fixed:
- test_pycvc_proxy_hooks: statically resolves every name in the generated
  pycvc/pycvc_gl proxies; fails on an undefined one (catches both hook
  shapes, including a future overload flipping a proxy's signature).
- cvcgl_stage_setters (runs in CI): every field of setStage/frameBounds/
  setKey/setWash/applyPreset sticks, and state -> object still flows.
- test_pycvc_gl_imgui: the uiScale round-trip only runs on a live overlay;
  an inert one (no CVC_ENABLE_IMGUI) is a documented no-op. The check never
  ran before -- the NameError was caught and reported as a SKIP.
@transfix
transfix merged commit 030aee1 into master Sep 30, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant