Group: Poor Abstraction Patterns

[kylemcdonald]

A poor abstraction is when a method or data belongs to one class, but should belong to a sub or super or adjacent class.

This is a group of issues identified during Code Review 2012. Add any relevant issues to this group, and when they are all closed we will close this issue. If an issue fits this pattern, but is more significant or not a simple fix, please add it as a separate issue.

[ofTheo]

OF KEY modifiers are GLUT specific https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofConstants.h#L480

need to make it easy to swap out with different window implementations.

[damian0815]

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/sound/ofFmodSoundPlayer.h#L28 ofFmodSoundStopAll() & co should be handled more cleanly.

[damian0815]

ofSoundPlayer includes loading code that should be/will be abstracted away to a separate sound file reader class, cf @arturoc ofSoundRefactoring branch eg https://github.com/arturoc/openFrameworks/blob/feature-ofSoundRefactoring/libs/openFrameworks/sound/ofSoundFile.h

[damian0815]

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/types/ofBaseTypes.h#L86 ofAbstractHasPixels seems unnecessary

[NickHardeman]

ofQuickTimePlayer ofQTKitPlayer

does not have getLoopState() function that is defined in ofVideoPlayer https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/video/ofVideoPlayer.h#L63

also, loop handling is inconsistent between ofSoundPlayer and ofVideoPlayer: ofBaseSoundPlayer api does not define getLoopState() at all.

[damian0815]

ofBaseRenderer has methods like enablePointSprites() that are too specific for a base class. should be replaced with a generic enable(ENUM) method to allow subclasses to extend in a flexible way.

[damian0815]

ofBaseRenderer setupGraphicDefaults() should perhaps be in ofStyle or window/app classes

[NickHardeman]

getLoopState() should return ofLoopType not int ofVideoPlayer https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/video/ofVideoPlayer.h#L63 ofBaseVideoPlayer https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/types/ofBaseTypes.h#L263 ofGstVideoPlayer https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/video/ofGstVideoPlayer.h#L45

[arturoc]

@damiannz ofAbstractHasPixels/ofAbstractPixels is actually used as a base class for anything that contains ofPixels_ of any type, since ofPixels is templated.

i would actually look into removing every base class which name is Has though, since it points to some kind of aggregation and seems useles if you can just pass the thing the class that extends it contains instead of the whole class

[damian0815]

ofPushStyle/ofPopStyle should be on ofBaseRenderer rather than floating around in ofStyle/globally

[danomatika]

The ofImage bind/unbind functions seem redundant to the same calls in ofTexture ... can't these be removed and the built in texture called instead via the reference:

myImage.getTextureReference().bind();

Same goes for ofImage::setCompression ...

Or they could at least be renamed ofImage::bindTexture(), ofImage::unbindTexture(), ofImage::setTextureCompression.

[damian0815]

in ofQuaternion::makeRotate( ofVec3f, ofVec3f ) there's a chunk of code that seems to efficiently calculate if a vector is normalized or not, without using a sqrt:

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/math/ofQuaternion.cpp#L80-86

this should be promoted to a function of ofVec2/3/4f, isNormalized() or isUnitLength() or similar

[danomatika]

What does ofPath::tessellate() do? Shouldn't it be a protected/private function?

[damian0815]

selecting sound APIs is inconsistently implemented -- we support selecting API in ofSoundStream via the listDevices/setDeviceId calls, but we don't do this for the soundPlayer. in ofFmodSoundPlayer on Linux, the output system is hardcoded to FMOD_OUTPUTTYPE_ALSA.

[damian0815]

ofOpenALSoundPlayer::setPan() has some nice constant panning code using sin and cos, should be promoted to ofSoundPlayer or somewhere more global as a helper function. https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/sound/ofOpenALSoundPlayer.cpp#L685-692

[underdoeg]

ofGraphics.cpp has a few opengl calls. Those should all be in ofGLRenderer so in theory of could run without opengl at some point. This concerns glu & glut headers. https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L10

GLUT is only used for the solid cone https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L1090

And I think glu is not used at all.

gl calls are used for ofDrawBitmapStringHighlight which can be moved to ofBitmapString https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L1122

and for ofBackgroundGradient which should be implemented in each renderer separately https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L337

[underdoeg]

@ofTheo

OF KEY modifiers are GLUT specific https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofConstants.h#L480 need to make it easy to swap out with different window implementations.

I don't think this is a problem. We'll have to settle for some value and you usually have to translate the window implementations keys for OF anyway.