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

README.md

@@ -120,9 +120,55 @@ Correctness and leaks:
 - PLY and STL text generation built one `NSString` per coordinate — 12 temporary
   objects per facet. Replaced by a single format pass producing byte-identical output.
 
-Not addressed: the mesh still draws from a client-side index array rather than an
-element buffer object, and point mode always draws all 307200 points regardless of the
-detail setting.
+Robustness:
+
+- All three export actions waited for a frame with `while (!depth) { depth = [self
+  createDepthData]; }`, which has no exit. With the device stopped or absent —
+  and the export menu items stay enabled in that state — `_depthUpdate` never turns
+  `YES`, so this pinned a core on the main thread and **froze the UI permanently**.
+  Replaced by `-waitForDepthDataUntil:`, a bounded wait that gives up after two
+  seconds and reports it in the status field.
+- `GLProgram.m` logged shader 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
+  but never printed.
+- `-[GLView dealloc]` released the display link without stopping it first, while its
+  callback still draws into the view, then ran `closeScene`'s `glDelete*` calls without
+  making the view's context current — deleting whatever context happened to be bound.
+- `_device` and `_halt` are each 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;
+  both are now `volatile` so that is explicit rather than accidental. The underlying
+  race is unchanged — a proper fix would use a condition variable.
+- Mesh indices now stream through an element buffer object instead of a client-side
+  array the driver must copy inline at draw time.
+
+Interface:
+
+- The export actions were never validated, so they stayed clickable with no device
+  running — the exact state in which they hung. `-validateMenuItem:` and
+  `-validateUserInterfaceItem:` now gray them out. AppKit validates menu and toolbar
+  items only; plain buttons in the nib still need their `enabled` state bound.
+- Deprecated AppKit calls replaced: `runModalForDirectory:file:` →
+  `setDirectoryURL:` + `runModal`, `NSOKButton` → `NSModalResponseOK`,
+  `NSShiftKeyMask` → `NSEventModifierFlagShift`, and `writeToFile:atomically:` →
+  the `encoding:error:` form. **This raises the deployment target well above the
+  10.6 the project declares** — it is a step toward building against a current SDK. It
+  will not compile against the 10.6 SDK the project still declares.
+- `setAllowedFileTypes:` is deprecated in favour of `allowedContentTypes`, which
+  needs the UniformTypeIdentifiers framework added to the project. Left as is rather
+  than editing the project file blind.
+
+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, but that cannot be settled without hardware, and parenthesising it would
+  freeze in one reading. Flagged, not touched.
+- Point mode always draws all 307200 points regardless of the detail setting. Changing
+  it would alter what the app renders.
+- `prepareOpenGL` and `reshape` do not call `super`, and the video offset properties
+  pair a synthesized setter with a hand-written getter under `atomic`. Both are warnings
+  on a rendering path that cannot be re-tested here.
 
 **None of this is verified against real hardware.** The changes compile clean under
 `clang -fsyntax-only` with the current macOS SDK, and the loop rewrites are covered by

src/AppDelegate.h

@@ -10,11 +10,15 @@ typedef struct {
 	float z;
 } VectorP;
 
-@interface AppDelegate : NSObject <NSApplicationDelegate> {    
+@interface AppDelegate : NSObject <NSApplicationDelegate, NSUserInterfaceValidations> {
 
 
-    freenect_device *_device;
-    BOOL _halt;
+    // Both are written on one thread and polled on the other with no barrier. The
+    // polling loops only work today because they call opaque functions (usleep,
+    // freenect_process_events) that force a reload; volatile makes that explicit
+    // instead of accidental.
+    freenect_device * volatile _device;
+    volatile BOOL _halt;
 
     NSString *_status;
 
@@ -71,6 +75,9 @@ typedef struct {
 - (uint16_t*)createDepthData;
 - (uint8_t*)createVideoData;
 
+// Bounded wait for the next frame. Returns NULL if none arrives in time.
+- (uint16_t*)waitForDepthDataUntil:(NSTimeInterval)seconds;
+
 // Optional counterparts to the create methods. Passing a buffer back here keeps it for
 // reuse; calling free() on it instead stays correct, it just loses the reuse.
 - (void)recycleDepthData:(uint16_t*)buffer;

src/AppDelegate.m

@@ -80,6 +80,34 @@ - (IBAction)play:(id)sender {
 }
 
 
+#pragma mark interface validation
+
+// The export actions need a live stream: with the device stopped they have nothing to
+// write and used to sit there spinning. Nothing grayed them out, so they stayed
+// clickable in exactly the state where they could not work. Covers menu items and
+// toolbar items; plain buttons in a window are not validated by AppKit and have to be
+// bound to the enabled state in the nib.
+- (BOOL)canExport {
+    return _device != NULL;
+}
+
+- (BOOL)validateAction:(SEL)action {
+    if(action == @selector(savePly:) || action == @selector(saveSTL:) || action == @selector(saveSTLB:)) {
+        return [self canExport];
+    }
+    return YES;
+}
+
+- (BOOL)validateMenuItem:(NSMenuItem*)item {
+    return [self validateAction:[item action]];
+}
+
+- (BOOL)validateUserInterfaceItem:(id<NSValidatedUserInterfaceItem>)item {
+    return [self validateAction:[item action]];
+}
+
+#pragma mark -
+
 - (double)rawDepthToMeters:(int) depthValue {
 	if(depthValue < _minDepth) depthValue = _minDepth;
 	if(depthValue > _maxDepth) depthValue = _maxDepth;
@@ -171,20 +199,21 @@ - (IBAction)savePly:(id)sender {
 	[NSThread sleepForTimeInterval:_snapdelay];
 
 
-	uint16_t *depth = [self createDepthData];
-		while (!depth) {
-			depth = [self createDepthData];
-		}
+	uint16_t *depth = [self waitForDepthDataUntil:2.0];
+	if(!depth) {
+		[self setStatus:@"No frame to export - is the device running?"];
+		return;
+	}
 	if(depth) {
-		int result;
+		NSModalResponse result;
 		NSArray *fileTypes = [NSArray arrayWithObject:@"ply"];
-		
+
 		NSSavePanel *oPanel = [NSSavePanel savePanel];
 		[oPanel setAllowedFileTypes:fileTypes];
-		
-		result = [oPanel runModalForDirectory:NSHomeDirectory()
-										 file:nil];
-		if (result == NSOKButton) {
+
+		[oPanel setDirectoryURL:[NSURL fileURLWithPath:NSHomeDirectory()]];
+		result = [oPanel runModal];
+		if (result == NSModalResponseOK) {
 			NSString *aFile = [[oPanel URL] path];
 
 			int stepx = _detailSlide, stepy = _detailSlide;
@@ -215,7 +244,7 @@ - (IBAction)savePly:(id)sender {
 		[PLYFile appendString:@"property float z\n"];
 		[PLYFile appendString:@"end_header\n"];
 		[PLYFile appendString:temps];
-		[PLYFile writeToFile:aFile atomically:YES];
+		[PLYFile writeToFile:aFile atomically:YES encoding:NSUTF8StringEncoding error:NULL];
 		[PLYFile release];
 		[temps release];
 
@@ -238,20 +267,21 @@ - (IBAction)saveSTL:(id)sender {
 
 	[NSThread sleepForTimeInterval:_snapdelay];
 
-	uint16_t *depth = [self createDepthData];
-	while (!depth) {
-		depth = [self createDepthData];
+	uint16_t *depth = [self waitForDepthDataUntil:2.0];
+	if(!depth) {
+		[self setStatus:@"No frame to export - is the device running?"];
+		return;
 	}
 	if(depth) {
-		int result;
+		NSModalResponse result;
 		NSArray *fileTypes = [NSArray arrayWithObject:@"STL"];
-		
+
 		NSSavePanel *oPanel = [NSSavePanel savePanel];
 		[oPanel setAllowedFileTypes:fileTypes];
-		
-		result = [oPanel runModalForDirectory:NSHomeDirectory()
-										 file:nil];
-		if (result == NSOKButton) {
+
+		[oPanel setDirectoryURL:[NSURL fileURLWithPath:NSHomeDirectory()]];
+		result = [oPanel runModal];
+		if (result == NSModalResponseOK) {
 			NSString *aFile = [[oPanel URL] path];
 
 			NSMutableString *temps2 = [[NSMutableString alloc] initWithString:@""];
@@ -294,7 +324,7 @@ - (IBAction)saveSTL:(id)sender {
 
 			[PLYFile appendString:temps2];
 			[PLYFile appendString:@"endsolid"];
-			[PLYFile writeToFile:aFile atomically:YES];
+			[PLYFile writeToFile:aFile atomically:YES encoding:NSUTF8StringEncoding error:NULL];
 			[PLYFile release];
 			[temps2 release];
 
@@ -319,21 +349,21 @@ - (IBAction)saveSTLB:(id)sender {
 	// the STL header field actually wants.
 	uint32_t faceCount = 0;
 	double nullb = 0x00;
-	uint16_t *depth = [self createDepthData];
-
-	while (!depth) {
-		depth = [self createDepthData];
+	uint16_t *depth = [self waitForDepthDataUntil:2.0];
+	if(!depth) {
+		[self setStatus:@"No frame to export - is the device running?"];
+		return;
 	}
 	if(depth) {
-		int result;
+		NSModalResponse result;
 		NSArray *fileTypes = [NSArray arrayWithObject:@"STL"];
-		
+
 		NSSavePanel *oPanel = [NSSavePanel savePanel];
 		[oPanel setAllowedFileTypes:fileTypes];
-		
-		result = [oPanel runModalForDirectory:NSHomeDirectory()
-										 file:nil];
-		if (result == NSOKButton) {
+
+		[oPanel setDirectoryURL:[NSURL fileURLWithPath:NSHomeDirectory()]];
+		result = [oPanel runModal];
+		if (result == NSModalResponseOK) {
 			NSString *aFile = [[oPanel URL] path];
 
 			NSMutableData *temps2 = [[[NSMutableData alloc] init] autorelease];
@@ -583,10 +613,12 @@ - (void)safeSetStatus:(NSString*)status {
 
 - (void)ioThread {
     freenect_context *_context;
+    freenect_device *dev = NULL;
     if(freenect_init(&_context, NULL) >= 0) {
         if(freenect_num_devices(_context) == 0) {
             [self safeSetStatus:@"No device"];
-        } else if(freenect_open_device(_context, &_device, 0) >= 0) {
+        } else if(freenect_open_device(_context, &dev, 0) >= 0) {
+            _device = dev; // opened through a local: &_device is no longer a plain freenect_device**
             freenect_set_user(_device, self);
             freenect_set_depth_callback(_device, depthCallback);
             freenect_set_video_callback(_device, rgbCallback);
@@ -714,6 +746,19 @@ - (uint8_t*)createVideoData {
     return src;
 }
 
+- (uint16_t*)waitForDepthDataUntil:(NSTimeInterval)seconds {
+    // The export paths used to spin on createDepthData with no exit condition. With the
+    // device stopped or absent _depthUpdate never turns YES, so that pinned a core on
+    // the main thread and froze the UI for good. Bounded wait, NULL on timeout.
+    uint16_t *depth = [self createDepthData];
+    NSDate *deadline = [NSDate dateWithTimeIntervalSinceNow:seconds];
+    while(!depth && [deadline timeIntervalSinceNow] > 0) {
+        usleep(2000);
+        depth = [self createDepthData];
+    }
+    return depth;
+}
+
 - (void)recycleDepthData:(uint16_t*)buffer {
     if(!buffer) return;
     @synchronized(self) {

src/GLProgram.m

@@ -22,7 +22,10 @@ -(GLuint)loadShader:(GLenum)type code:(const char *)code {
     GLint status;
 	glGetShaderiv(shader, GL_COMPILE_STATUS, &status);
 	if(status == 0)
-		NSLog(@"Failed to compile desc: %s", desc, code);
+		// desc is an NSString and was being passed to %s, which read the object pointer
+		// as a C string - garbage or a crash on the one path that reports shader errors.
+		// code was passed but never printed.
+		NSLog(@"Failed to compile %@:\n%s", desc, code);
 
 	return shader;
 }

src/GLView.h

@@ -15,7 +15,8 @@
 
     GLuint _depthTex, _videoTex, _colormapTex;
     GLuint _pointBuffer;
-
+    GLuint _indexBuffer;
+
     GLuint *_indicies;
     int _nTriIndicies;
 

src/GLView.m

@@ -68,7 +68,16 @@ - (id) initWithFrame: (NSRect) frame
 }
 
 - (void)dealloc {
-    CVDisplayLinkRelease(_displayLink);
+    // Stop the link before releasing it: its callback draws into this view, so it has
+    // to be off before the scene goes away. And closeScene deletes GL objects, which
+    // needs this view's context to be the current one - it was deleting whatever
+    // happened to be bound on this thread, or nothing at all.
+    if(_displayLink) {
+        CVDisplayLinkStop(_displayLink);
+        CVDisplayLinkRelease(_displayLink);
+        _displayLink = NULL;
+    }
+    [[self openGLContext] makeCurrentContext];
     [self closeScene];
     [super dealloc];
 }
@@ -344,6 +353,7 @@ - (void)initScene {
     // create indicies for mesh
     _indicies = (GLuint*)malloc(FREENECT_FRAME_W*FREENECT_FRAME_H*6*sizeof(GLuint));
     _nTriIndicies = 0;
+    glGenBuffers(1, &_indexBuffer);
 
     _offset[0] = 0;
     _offset[1] = 0;
@@ -365,7 +375,8 @@ - (void)closeScene {
     glDeleteTextures(1, &_videoTex);
     glDeleteTextures(1, &_colormapTex);
     glDeleteBuffers(1, &_pointBuffer);
-
+    glDeleteBuffers(1, &_indexBuffer);
+
     free(_indicies);
 
     [_depthProgram release];
@@ -534,8 +545,18 @@ - (void)drawScene {
         glVertexPointer(2, GL_FLOAT, 0, NULL);
         if(mode == MODE_POINTS) {
             glDrawArrays(GL_POINTS, 0, FREENECT_FRAME_W*FREENECT_FRAME_H);
-        } else {
-            glDrawElements(GL_TRIANGLES, _nTriIndicies, GL_UNSIGNED_INT, _indicies);
+        } else if(_nTriIndicies > 0) {
+            // Stream the indices through a buffer object rather than handing the driver
+            // a client-side array it has to copy inline at draw time. The NULL
+            // glBufferData first orphans the previous storage, so this upload does not
+            // wait on the frame still reading the old one.
+            glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, _indexBuffer);
+            glBufferData(GL_ELEMENT_ARRAY_BUFFER, _nTriIndicies*sizeof(GLuint), NULL, GL_STREAM_DRAW);
+            glBufferData(GL_ELEMENT_ARRAY_BUFFER, _nTriIndicies*sizeof(GLuint), _indicies, GL_STREAM_DRAW);
+            glDrawElements(GL_TRIANGLES, _nTriIndicies, GL_UNSIGNED_INT, NULL);
+            // Must unbind: drawFrustrum below draws from a client-side index array, and
+            // with an element buffer still bound its pointer would be read as an offset.
+            glBindBuffer(GL_ELEMENT_ARRAY_BUFFER, 0);
         }
         glBindBuffer(GL_ARRAY_BUFFER, 0);
         glDisableClientState(GL_VERTEX_ARRAY);
@@ -631,7 +652,7 @@ - (void)mouseDragged:(NSEvent*)event {
     NSPoint delta = NSMakePoint((pos.x-_lastPos.x)/[self bounds].size.width, (pos.y-_lastPos.y)/[self bounds].size.height);
     _lastPos = pos;
 
-    if([event modifierFlags] & NSShiftKeyMask) {
+    if([event modifierFlags] & NSEventModifierFlagShift) {
         _offset[0] += 2*delta.x;
         _offset[1] += 2*delta.y;
     } else {
@@ -642,7 +663,7 @@ - (void)mouseDragged:(NSEvent*)event {
 
 - (void)scrollWheel:(NSEvent *)event {
     if([ctrl mode] == MODE_2D) return;
-    float d = ([event modifierFlags] & NSShiftKeyMask) ? [event deltaX] : [event deltaY];
+    float d = ([event modifierFlags] & NSEventModifierFlagShift) ? [event deltaX] : [event deltaY];
     _offset[2] += d*0.1;
     if(_offset[2] < 0.5) _offset[2] = 0.5;
 }