Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Group: Poor Abstraction Patterns #1548

Open
kylemcdonald opened this Issue Sep 6, 2012 · 17 comments

Comments

Projects
None yet
7 participants
Contributor

kylemcdonald commented Sep 6, 2012

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.

Contributor

ofTheo commented Sep 6, 2012

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.

Contributor

damian0815 commented Sep 6, 2012

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

Contributor

NickHardeman commented Sep 6, 2012

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.

Contributor

damian0815 commented Sep 6, 2012

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.

Contributor

damian0815 commented Sep 6, 2012

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

Owner

arturoc commented Sep 6, 2012

@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

Contributor

damian0815 commented Sep 6, 2012

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

Contributor

danomatika commented Sep 6, 2012

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.

Contributor

damian0815 commented Sep 6, 2012

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

Contributor

danomatika commented Sep 6, 2012

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

Contributor

damian0815 commented Sep 6, 2012

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.

Contributor

damian0815 commented Sep 6, 2012

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

Contributor

underdoeg commented Sep 6, 2012

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

Contributor

underdoeg commented Sep 7, 2012

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment