diff --git a/README.md b/README.md index 36c4a44..844fb41 100644 --- a/README.md +++ b/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 diff --git a/src/AppDelegate.h b/src/AppDelegate.h index 9410a98..b19224e 100644 --- a/src/AppDelegate.h +++ b/src/AppDelegate.h @@ -10,11 +10,15 @@ typedef struct { float z; } VectorP; -@interface AppDelegate : NSObject { +@interface AppDelegate : NSObject { - 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; diff --git a/src/AppDelegate.m b/src/AppDelegate.m index a437d81..59ed4b0 100644 --- a/src/AppDelegate.m +++ b/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)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) { diff --git a/src/GLProgram.m b/src/GLProgram.m index 85442c3..204f909 100644 --- a/src/GLProgram.m +++ b/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; } diff --git a/src/GLView.h b/src/GLView.h index 416d4bc..30af70e 100644 --- a/src/GLView.h +++ b/src/GLView.h @@ -15,7 +15,8 @@ GLuint _depthTex, _videoTex, _colormapTex; GLuint _pointBuffer; - + GLuint _indexBuffer; + GLuint *_indicies; int _nTriIndicies; diff --git a/src/GLView.m b/src/GLView.m index 0a80465..8111505 100644 --- a/src/GLView.m +++ b/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; }