Skip to content

Step 3: stop re-uploading GPU buffers while dragging a session's gizmo - #553

Draft
michalpelka wants to merge 11 commits into
MapsHD:mainfrom
michalpelka:mp/step3-fix-gizmo-drag-gpu-churn
Draft

Step 3: stop re-uploading GPU buffers while dragging a session's gizmo#553
michalpelka wants to merge 11 commits into
MapsHD:mainfrom
michalpelka:mp/step3-fix-gizmo-drag-gpu-churn

Conversation

@michalpelka

@michalpelka michalpelka commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Dragging a session's gizmo in step 3 mutated every scan's m_pose every frame, which forced ScanRenderer::syncPoses() to tear down and re-upload each scan's full GPU point buffer every single frame for as long as the drag lasted (visible as a stream of VAO: [ID ...] Unloaded... log lines while dragging). Merely selecting a gizmo (not even dragging) triggered the same rebuild, because a double -> float -> double round trip through the gizmo matrix was enough float noise to look like a pose change every frame.
  • While ImGuizmo::IsUsing() is true, the candidate pose chain is now stashed in session_drag_preview_poses instead of being written into the session; the render loop draws it via ScanRenderer::drawCachedWithTransform() (pose folded into the MVP, no GPU re-upload) until the drag ends, at which point the pose is committed for real — the only frame that triggers an actual rebuild.
  • drawCachedWithTransform() only supported a flat/intensity bool, so the drag preview rendered flat-colored instead of matching the scene's live per-point shading (View > Points color, elevation/distance bounds). It now takes the real ScanColorMode plus the elevation/distance bounds those modes need, defaulting to Flat so the existing loop-closure edge preview callers (step 2 and step 3) keep their current look; the drag preview opts in via renderScanAtPose()'s new useSceneColorMode flag.
  • Removed ~200 lines of dead, commented-out pre-raylib-port code in multi_session_registration.cpp's display() (referenced a session variable and GLUT/legacy-GL calls no longer present in this file).

Test plan

  • multi_session_registration_step_3 and multi_view_tls_registration_step_2 both build cleanly (RelWithDebInfo, clang-18).
  • Manually verified in step 3 against a real multi-session project: dragging a session's gizmo no longer prints VAO: [ID ...] Unloaded... per frame, and the dragged session now shades the same way as the rest of the scene instead of going flat.
  • Would be good for a second person to sanity-check gizmo_all_sessions (move-everything) on a multi-session project before merging.

🤖 Generated with Claude Code

michalpelka and others added 10 commits October 2, 2026 14:55
@michalpelka @claude
Step 3: stop re-uploading GPU buffers while dragging a session's gizmo
141faec 
Dragging a session's gizmo mutated every one of its scans' m_pose every
frame, which made ScanRenderer::syncPoses() tear down and re-upload each
scan's full point buffer every frame for as long as the drag lasted.
Merely having a gizmo selected (not dragging) also triggered this: a
double -> float -> double round trip through the gizmo matrix was enough
float noise to look like a pose change every frame.

While ImGuizmo::IsUsing() is true, the session's candidate pose chain is
now stashed in session_drag_preview_poses instead of being written into
the session, and the render loop draws it via
ScanRenderer::drawCachedWithTransform() (pose folded into the MVP, no
GPU re-upload) until the drag ends, at which point the pose is committed
for real -- the only frame that triggers a rebuild.

drawCachedWithTransform() took a flat/intensity bool, so the drag
preview rendered flat-colored instead of matching the live per-point
shading (View > Points color, elevation/distance bounds) the rest of
the scene uses. It now takes the real ScanColorMode (plus the
elevation/distance bounds Elevation/Distance need), defaulting to Flat
so the existing loop-closure edge preview callers (step 2 and step 3)
are unaffected; the drag preview opts in via renderScanAtPose()'s new
useSceneColorMode flag.

Also removes ~200 lines of dead, commented-out pre-raylib-port code in
multi_session_registration.cpp's display() that referenced a `session`
variable and GLUT/legacy-GL calls no longer present in this file.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: add a slider for index_active_edge
e1e010a 
The +/- InputInt was the only way to change the active loop-closure
edge, which is painfully slow with many edges on a large project.
Adds a SliderInt next to it for quick jumps.

Fixes MapsHD#549.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: add sliders for index_loop_closure_source/target
35bded7 
Same InputInt-only problem as index_active_edge (MapsHD#549): no fast way to
scrub to a far-away scan index when setting up a loop closure.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: put the new sliders on the same line as their InputInt
3aa3db0 
SameLine() between slider and input, matching num_edge_extended_before/after's existing layout, instead of each on its own row.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: add View > Intersections menu (xz/yz/xy cross-section slicing)
62f46b8 
Ported from step 2's per-session "Intersections" menu. Step 3 has no
single active session, so (same pattern as View > Points size) the
width and three toggles are global and get pushed to every loaded
session's point_clouds_container when changed. The renderer already
read these fields (ScanRenderer's xzOn/yzOn/xyOn shader uniforms); step
3 just had no UI for them.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: fix ICP ground-truth branch using un-reordered source session
312acc1 
In every "ICP" button's is_with_ground_truth branch, `source` was built
from edges[index_active_edge]'s raw index_session_to/index_to instead of
the already-reordered index_session_to/index_to locals. Those locals get
swapped when the *target* (not source) side of the edge is the ground
truth session; source wasn't tracking that swap, so ICP silently aligned
the ground-truth scan against a filtered copy of itself whenever ground
truth was on the edge's "to" side, and the registration error grew with
every iteration. Pre-existing bug, not introduced by this branch.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Share save_intersection() via core/export_laz.h
3e626aa 
Step 2's save_intersection() (per-session xz/yz/xy slab export) only
existed in multi_view_tls_registration.cpp, so step 3 couldn't reuse it
without copy-pasting the filter loop. Move it into Core/export_laz.h
(already included by both apps) as the one shared implementation.

Also fixes a latent array-desync bug while moving it: a point past the
end of p.timestamps was pushed to `intensity` and `pointcloud` but not
to `timestamps`, leaving the three parallel arrays different lengths.
Now defaults to 0.0, matching the existing save_all_to_las() pattern in
the same header.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Step 3: intersections apply to loop closure/drag preview; grid overla…
e1074d2 
…ys + export

- drawCachedWithTransform() now sets the xz/yz/xy slab-filter uniforms from
  the caller instead of hardcoding them off, so the loop-closure edge view
  and the gizmo-drag preview respect View > Intersections like the main
  render does.
- Consolidated the loop-closure panel's separate "gui_point_size" (which
  unconditionally overwrote every session's point_size every frame,
  fighting View > Points size) into the one point_size control.
- Added step 2's remaining Intersections-menu features: 10m/1m/0.1m grid
  overlays per xz/yz/xy plane (drawIntersectionGrids(), ported to rlgl) and
  "Export xz/yz/xy intersection" (one laz per session, auto-named -- step 3
  has no single active session to pick a file dialog for). Moved the menu
  to the top level to match step 2's ordering.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Fix ambiguous save_intersection() call in legacy step 2 app
2cc990c 
The legacy GLUT step 2 app (forked in 39ffca5) kept its own
save_intersection() in multi_view_tls_registration.{h,cpp}, left over
from before 3e626aa moved the shared implementation into
Core/export_laz.h. Since the legacy gui.cpp already includes that
header too, both overloads were visible and MSVC failed to pick one
(C2668), breaking the Windows CI build.

Drop the legacy duplicate so it shares the same Core/export_laz.h
implementation as the raylib-based step 2 app.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka @claude
Add HDMAPPING_ASSERT: a loud invariant check that survives NDEBUG
5a72dc7 
Unlike assert(), it is never compiled out by a release build, so a failed
invariant is never silently swallowed. Debug builds abort immediately;
release builds log and continue by default, since aborting a shipped app on
an invariant violation is worse for users than limping on in a degraded
state. The new HDMAPPING_ASSERT_ALWAYS_ABORT CMake option forces the abort
even with NDEBUG defined, for debugging a RelWithDebInfo/Release build
without flipping NDEBUG (and everything else gated on it) project-wide.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka
michalpelka marked this pull request as draft October 5, 2026 15:12
@michalpelka
michalpelka force-pushed the mp/step3-fix-gizmo-drag-gpu-churn branch from d3aa9e1 to 3fa8825 Compare October 5, 2026 15:24
@michalpelka @claude
Step 3: fix edge-activation crashes and loop-closure open/close lifec…
6432f6d 
…ycle

Harden the manual loop-closure edge list against the crashes hit while
selecting edges with only some sessions visible:

- canEdgeBeActivated() checked the global index_active_edge instead of its
  own parameter, and its bounds guard was inverted, so it fell through to
  edges.at(index_active_edge) precisely when that index was invalid (-1 on
  the common "no edge selected yet" path) -- guaranteed std::out_of_range.
  It also checked index_from/index_to (scan indices) against
  visible_sessions, which holds session indices. Now it checks the passed-in
  edge index's own index_session_from/to, bounds-safely.
- setActiveEdge() asserted visible_sessions.contains(first_session_index) /
  second_session_index -- the previously active edge's sessions, still
  stale at that point -- instead of the sessions of the edge actually being
  activated. Reordered to read the edge once and validate everything
  against it before any assignment.
- settings_gui()'s per-frame visibility sync inserted the stale
  first_session_index into visible_sessions instead of the loop's own
  session index, so visible_sessions (a set) collapsed to at most one
  lagging entry instead of the real set of visible sessions -- undermining
  every check above that gates on it.
- display()'s manipulate_active_edge rendering block mixed the same
  first_session_index/second_session_index globals (resynced every frame by
  settings_gui() for the unrelated "pick two sessions for a new edge" UI)
  with edges[index_active_edge]'s own session fields: an index range
  bounds-checked against one session's point-cloud count was then used to
  index a *different* session via .at(), e.g. walking a range sized for a
  576-scan session while indexing a 177-scan one. Introduced
  active_session_from/active_session_to read once from the active edge and
  used consistently, matching dead commented-out code right above it that
  already did this correctly. Also fixed an adjacent bounds check comparing
  first_session_index itself (a session index) against a point-cloud count
  instead of the scan index (index_loop_closure_source) actually used.
- The "Set Active" button's BeginDisabled/EndDisabled wrapped the
  already-executed click body instead of the button itself, so it never
  actually blocked anything; moved it to wrap the button.

Also fix on_loop_closure_gui_open/close() never firing when the loop-closure
window is closed via its own title-bar X: that flips is_loop_closure_gui
from inside the very call the old code checked before, so the transition
was always missed. The close check now runs after loop_closure_gui() so it
observes the post-click value. Factor the five duplicated "two sessions
ground truth -> skip, else trim ground truth cloud to bbox" ICP button
bodies into shared icp_active_edge()/ground_truth_icp_source_target()
helpers, and link spdlog::spdlog explicitly (it was only reaching this
target transitively via core's PRIVATE link, without the fmt dependency
spdlog's compiled logger needs).

main()'s outer catch (const std::exception&)/catch (...) around the whole
app silently swallowed any uncaught exception -- including exactly the bugs
above -- printed e.what() with no trailing newline, and returned 0, so a
real invariant violation looked like a clean exit. Removed both, keeping
only the bad_alloc handler (a deliberate, distinct OOM dialog); any other
uncaught exception now reaches std::terminate() and aborts loudly, matching
HDMAPPING_ASSERT's "never silently continue" design.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@michalpelka
michalpelka force-pushed the mp/step3-fix-gizmo-drag-gpu-churn branch from 3fa8825 to 6432f6d Compare October 5, 2026 15:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@michalpelka