Skip to content

fix: unfreeze the export paths, repair shader error logging, validate the UI - #2

Closed
gridboy wants to merge 3 commits into
perf/render-hot-pathsfrom
fix/robustness-and-ui
Closed

gridboy wants to merge 3 commits into
perf/render-hot-pathsfrom
fix/robustness-and-ui

Conversation

@gridboy

@gridboy gridboy commented Aug 11, 2026

Copy link
Copy Markdown
Owner

Stacked on #1. Base is perf/render-hot-paths, so this PR shows only its own two commits. Merge #1 first, then this retargets to main automatically.

What this changes

Fixes a permanent UI freeze, a crash on the shader error path, unsound teardown, and the missing interface validation that made the freeze reachable in the first place.

Why

The freeze. All three export actions waited for a frame like this:

while (!depth) { depth = [self createDepthData]; }

There is no exit. With the device stopped or absent, _depthUpdate never turns YES, so this pinned a core on the main thread and froze the UI for good — force quit only. Nothing grayed the export menu items out, so they stayed clickable in exactly the state where they could not work. Two clicks to reach. Now a bounded wait (-waitForDepthDataUntil:) that gives up after two seconds and says so in the status field, plus -validateMenuItem: / -validateUserInterfaceItem: so the actions gray out. AppKit validates menu and toolbar items only — plain buttons in the nib still need their enabled state bound there.

Shader errors. GLProgram.m logged compile failures with NSLog(@"...%s", desc, code) where desc is an NSString *. %s read the object pointer as a C string: garbage output or a crash, on the single path whose job is to report shader errors. code was passed and never printed. Surfaced by -Wall.

Teardown. -[GLView dealloc] released the display link without stopping it first, while its callback still draws into the view, then let closeScene issue glDelete* calls without making the view's context current — deleting whatever context happened to be bound on that thread.

Threading. _device and _halt are each written on one thread and polled on the other with no barrier. Both are now volatile. This does not fix the race, it makes explicit what the polls already rely on: opaque calls (usleep, freenect_process_events) forcing a reload. A real fix wants a condition variable.

Deprecated AppKit. runModalForDirectory:file: → setDirectoryURL: + runModal, NSOKButton → NSModalResponseOK, NSShiftKeyMask → NSEventModifierFlagShift, writeToFile:atomically: → the encoding:error: form. This raises the effective deployment target well above the 10.6 the project declares — a step toward a current SDK, not something Xcode 3 will still compile. Worth a decision before merging.

Also: mesh indices now stream through an element buffer object instead of a client-side array the driver copies inline at draw time. Note the unbind before drawFrustrum, which still draws from a client array and would otherwise have its pointer read as an offset.

How it was verified

  • sh scripts/syntax-check.sh passes; GLProgram.m is now warning-clean
  • Behaviour-preserving comparison — not applicable, these are behaviour changes
  • Performance claims — the element buffer change is unmeasured, GPU-side
  • Built in Xcode
  • Run against real Kinect hardware

Unlike #1, none of this is backed by observed behaviour — it is reasoning over the code plus what -Wall reports. The freeze, the %s crash and the teardown ordering are each clear from the source, but a reviewer with hardware should confirm the export paths and the mesh still render.

Left alone deliberately

  • saveSTLB has a face-detection condition mixing && and || without parentheses (-Wlogical-op-parentheses). The precedence the compiler applies may not be what the author meant; that cannot be settled without hardware, and parenthesising it would freeze one reading. Flagged, untouched.
  • Point mode draws all 307200 points regardless of the detail setting. Changing it alters what the app renders.
  • setAllowedFileTypes: is deprecated in favour of allowedContentTypes, which needs the UniformTypeIdentifiers framework wired into the Xcode project.
  • prepareOpenGL and reshape do not call super; the video offset properties pair a synthesized setter with a hand-written getter under atomic.

Provenance

  • No third-party licence header, copyright notice, CONTRIB or AUTHORS file altered
  • No new vendored code

…rdown

- The three export actions waited for a frame with an unbounded 'while (!depth)'
  spin. With the device stopped or absent - the menu items stay enabled then -
  _depthUpdate never turns YES, so this pinned a core on the main thread and froze
  the UI for good. Replaced by -waitForDepthDataUntil:, which gives up after two
  seconds and reports it in the status field.

- GLProgram logged shader compile failures with NSLog(@"...%s", desc, code) where
  desc is an NSString*. %s read the object pointer as a C string: garbage or a crash
  on the one path meant to report shader errors. code was never printed.

- -[GLView dealloc] released the display link without stopping it while its callback
  still draws into the view, then let closeScene issue glDelete* calls with no current
  context.

- Mark _device and _halt volatile. Each is written on one thread and polled on the
  other with no barrier; the polls only work because they call opaque functions that
  force a reload. The race itself is unchanged.

- Stream mesh indices through an element buffer object rather than a client-side array
  copied inline at draw time, unbinding before drawFrustrum, which still draws from a
  client array and would otherwise read its pointer as an offset.

Verified with clang -fsyntax-only -Wall against the current macOS SDK: no errors, and
GLProgram is now warning-clean. The remaining warnings are documented in the README as
deliberately untouched. Still not built or run against Kinect hardware.
The export menu items were never validated, so they stayed clickable with no device
running - the one state in which they had nothing to write and used to hang. Add
-validateMenuItem: and -validateUserInterfaceItem: so they gray out. AppKit validates
menu and toolbar items only; plain buttons in the nib still need their enabled state
bound there.

Replace AppKit calls that no longer exist or are deprecated:
- runModalForDirectory:file: -> setDirectoryURL: + runModal
- NSOKButton -> NSModalResponseOK
- NSShiftKeyMask -> NSEventModifierFlagShift
- NSString writeToFile:atomically: -> writeToFile:atomically:encoding:error:

This raises the effective deployment target well above the 10.6 the project declares.
It is a step toward building against a current SDK, not something Xcode 3 will still
compile. setAllowedFileTypes: is left alone: allowedContentTypes needs the
UniformTypeIdentifiers framework wired into the project file.

clang -fsyntax-only -Wall against the current SDK: no errors, and no new warnings
beyond the ones already documented as deliberately untouched.
Says what is true about the SDK rather than naming the IDE: the modernised AppKit calls
will not compile against the 10.6 SDK the project declares, and the UniformTypeIdentifiers
note refers to the project file.
@gridboy
gridboy deleted the branch perf/render-hot-paths August 11, 2026 13:50
@gridboy gridboy closed this Aug 11, 2026
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