Phase 1: Main loop cleanup — reorder phases, separate concerns

[brendancol]

Parent: #57 — Phase 1 Architecture

Summary

Clean up the main loop by reordering phases (input before update), separating concerns in _tick() and _update_frame(), and fixing the one-frame input lag in GLFW mode. No new class hierarchies — just reorganization of existing code.

Why not a GameLoop class with fixed timestep and Platform ABC?

The original proposal introduced a GameLoop class with an accumulator-based fixed-timestep pattern, and a Platform ABC with GLFWPlatform / JupyterPlatform subclasses. Both are over-engineered for the current state:

1. Fixed timestep solves a physics problem rtxpy doesn't have. The accumulator pattern (while accumulator >= fixed_dt) ensures deterministic physics simulation regardless of frame rate. rtxpy doesn't simulate physics — camera movement is delta-time scaled, which is correct for a visualization tool. The existing dt * speed approach gives smooth movement at any frame rate. Fixed timestep would only matter if we added rigid body dynamics (Phase 3), and it can be added then.

2. Two backends don't justify an ABC. There are exactly two platforms (GLFW and Jupyter) with very different models (callback-based vs. queue-based, OpenGL texture upload vs. JPEG-over-widget). An ABC adds a layer of indirection that makes both harder to understand, and there's no third platform on the horizon. If one appears, the abstraction can be extracted then.

3. Variable render rate already works. The current loop renders only when _render_needed is true and _frame_dirty gates texture upload. This is already the "render only when dirty" pattern the proposal describes — it just needs clearer phase separation, not a new class.

What IS needed: the loop has real bugs and organizational problems that are worth fixing without new abstractions.

Current State

GLFW Desktop Loop (engine.py:6206-6243)

while not glfw.window_should_close(window) and self.running:
    _tick()                          # Update (includes held-key input processing)
    [drain REPL command queue]       # Command processing
    [upload + render if dirty]       # Present
    glfw.poll_events()               # Input polling ← AFTER render!
    [optional sleep if idle]

Bug: one-frame input lag. glfw.poll_events() runs AFTER _tick() and rendering. This means key presses and mouse events from the current frame aren't processed until the next frame's _tick(). Moving poll_events() to the top of the loop fixes this.

_tick() method (engine.py:3939-3966+)

Mixes three concerns:

1. Input processing: delta-time computation, held-key → movement accumulation (lines 3944-3955)
2. Simulation: chunk manager update, wind particle replay (lines 3946+)
3. Render decisions: AO/DOF accumulation state, _render_needed triggering

_update_frame() method (engine.py:5114-5238)

Mixes two concerns:

1. Render: GPU ray trace, progressive accumulation, post-processing
2. Present: sync previous GPU readback, async D2H copy, overlay compositing

Jupyter Loop (notebook.py:329-371)

while self.running:
    [drain input queue]              # Input
    [drain REPL command queue]       # Commands
    _tick()                          # Update
    [push JPEG if dirty]             # Present
    [sleep to target 15 FPS]         # Rate limiting

The Jupyter loop already has better phase ordering (input before tick), but mixes rate limiting with presentation.

Proposed Changes

1. Move glfw.poll_events() before _tick()

Single-line fix that eliminates the one-frame input lag:

while not glfw.window_should_close(window) and self.running:
    glfw.poll_events()               # Input FIRST
    self._drain_command_queue()      # REPL commands
    self._tick()                     # Update (uses fresh input)
    self._present_if_dirty()         # Upload texture + swap
    if not self._held_keys and not self._mouse_dragging:
        time.sleep(0.008)

2. Split _tick() into clear sections

Not new methods for the sake of methods — just clear section comments and grouping:

def _tick(self):
    # --- Delta time ---
    now = time.monotonic()
    dt = min(now - self._last_tick_time, 0.1)
    self._last_tick_time = now
    dt_scale = dt / 0.05

    # --- Input → camera movement ---
    if self.input.held_keys:
        self.camera.apply_input(self.input.held_keys, dt_scale)
        self._render_needed = True

    # --- Simulation updates ---
    self.geometry.load_chunks_near(self.camera.position)
    self.wind.step(dt)

    # --- Render if needed ---
    if self._render_needed:
        self._update_frame()
        self._render_needed = False

This aligns with the composition approach from #61 — _tick() delegates to subsystem objects instead of inlining everything.

3. Extract _drain_command_queue() and _present_if_dirty()

Pull the inline REPL drain and texture upload into named methods for readability:

def _drain_command_queue(self):
    """Process REPL commands queued from the ViewerProxy thread."""
    while True:
        try:
            cmd = self._command_queue.get_nowait()
        except queue.Empty:
            break
        try:
            cmd(self)
        except Exception:
            traceback.print_exc()
        self._render_needed = True

def _present_if_dirty(self):
    """Upload frame texture and swap buffers if a new frame is ready."""
    if self._frame_dirty and self._display_frame is not None:
        # ... existing texture upload + swap_buffers code ...
        self._frame_dirty = False

4. Split _update_frame() render vs. present concerns

The async GPU readback naturally separates render from present:

def _update_frame(self):
    # --- Sync previous frame's async readback ---
    if self._pending_readback is not None:
        self._sync_readback()

    # --- Render: ray trace + post-process ---
    self._launch_optix()
    self._post_process()  # AO, DOF, denoise, tonemap

    # --- Kick async readback for this frame ---
    self._start_async_readback()

    # --- Composite overlays onto display frame ---
    self._composite_overlays()
    self._frame_dirty = True

What NOT to change

• No fixed timestep accumulator. Delta-time scaling works correctly for camera movement. Add fixed timestep only when/if physics simulation is added (Phase 3).
• No Platform ABC. GLFW and Jupyter loops stay as separate code paths. They share _tick() and _update_frame() but their loop structures are different enough that an ABC would be forced.
• No GameLoop class. The loop is 15 lines of code in a try/finally block. Wrapping it in a class adds indirection without value.
• No delta-time refactor. The current dt_scale = dt / 0.05 reference-rate pattern is fine — it gives consistent movement speed. Changing to raw dt * speed_in_units_per_second would require retuning all speed defaults for no user-visible benefit.

Implementation Notes

• The glfw.poll_events() reorder is a one-line move with immediate benefit — can land independently
• _tick() reorganization depends on Phase 1: Decompose InteractiveViewer via Composition #61 (composition) for the subsystem delegation calls
• _drain_command_queue() extraction is a pure refactor — no behavior change
• _present_if_dirty() extraction is a pure refactor — no behavior change
• _update_frame() split is the most involved change but follows the existing async readback boundary
• Jupyter loop (notebook.py) benefits from the same _tick() cleanup but its loop structure stays separate

Acceptance Criteria

• glfw.poll_events() called before _tick() — no one-frame input lag
• _tick() has clear input/simulation/render sections (comments or subsystem delegation)
• REPL command drain extracted to _drain_command_queue()
• Texture upload + swap extracted to _present_if_dirty()
• _update_frame() separates render (ray trace + post-process) from present (readback + composite)
• Existing explore() functionality preserved — no user-visible behavior change
• Camera movement speed unchanged at any frame rate