Group: Cleanup #1554

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

Comments

Projects
None yet
9 participants
@kylemcdonald
Contributor

kylemcdonald commented Sep 6, 2012

Cleanup includes anything that needs to just be removed or rearranged/moved around without changing the syntax or semantics of openFrameworks. For example: comment removal, or moving definitions around.

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.

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofTheo

This comment has been minimized.

Show comment Hide comment
@ofTheo

This comment has been minimized.

Show comment Hide comment
@ofTheo

ofTheo Sep 6, 2012

Contributor
Contributor

ofTheo commented Sep 6, 2012

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

ofZach Sep 6, 2012

Contributor
@kylemcdonald

This comment has been minimized.

Show comment Hide comment
@kylemcdonald

kylemcdonald Sep 6, 2012

Contributor

ofMatrix4x4.h has a line that reads ofVec3f c = rotateZ 30º ofVec3f a around ofVec3f b and the degree symbol is an extended ASCII character, which should be removed.

Contributor

kylemcdonald commented Sep 6, 2012

ofMatrix4x4.h has a line that reads ofVec3f c = rotateZ 30º ofVec3f a around ofVec3f b and the degree symbol is an extended ASCII character, which should be removed.

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

ofZach Sep 6, 2012

Contributor
@kylemcdonald

This comment has been minimized.

Show comment Hide comment
@kylemcdonald

kylemcdonald Sep 6, 2012

Contributor

there's a random line in ofMatrix4x4.h that says AARON METHOD without explanation that should be removed.

Contributor

kylemcdonald commented Sep 6, 2012

there's a random line in ofMatrix4x4.h that says AARON METHOD without explanation that should be removed.

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

ofZach Sep 6, 2012

Contributor

should only have one implementation of translate and setColor. wrapping happens at ofTranslate level:

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofRendererCollection.h

Contributor

ofZach commented Sep 6, 2012

should only have one implementation of translate and setColor. wrapping happens at ofTranslate level:

https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofRendererCollection.h

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The ofGetGlInternalFormat, ofGetGlFormatAndType, & ofGetImageTypeFromGLType functions in ofTexture could be static since they don't appear to change any internal data.

Contributor

danomatika commented Sep 6, 2012

The ofGetGlInternalFormat, ofGetGlFormatAndType, & ofGetImageTypeFromGLType functions in ofTexture could be static since they don't appear to change any internal data.

@ofTheo

This comment has been minimized.

Show comment Hide comment
@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

O/C: ofVec4f operator+/+=/-/-= etc could be ordered more nicely

Contributor

damian0815 commented Sep 6, 2012

O/C: ofVec4f operator+/+=/-/-= etc could be ordered more nicely

@NickHardeman

This comment has been minimized.

Show comment Hide comment
@NickHardeman

NickHardeman Sep 6, 2012

Contributor

ofNode getGlobalScale has some commented out code that should be removed
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/3d/ofNode.cpp#L305-308

Contributor

NickHardeman commented Sep 6, 2012

ofNode getGlobalScale has some commented out code that should be removed
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/3d/ofNode.cpp#L305-308

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

This comment in ofTexture refers to immediate mode. Are we still using intermediate mode in this case anymore?

Contributor

danomatika commented Sep 6, 2012

This comment in ofTexture refers to immediate mode. Are we still using intermediate mode in this case anymore?

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

Could getCoordFromPoint & getCoordFromPercent in ofTexture be static? They don't seem to work wiht internal data.

Contributor

danomatika commented Sep 6, 2012

Could getCoordFromPoint & getCoordFromPercent in ofTexture be static? They don't seem to work wiht internal data.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

ofCairoRender::setup takes alot of variables ... and alot of these are defaults.

Contributor

danomatika commented Sep 6, 2012

ofCairoRender::setup takes alot of variables ... and alot of these are defaults.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The following variables in ofImage should probably be protected/private:

  • width
  • height
  • bpp
  • type

They should be replaced with getters:

  • getWidth()
  • getHeight()
  • getBpp()
  • getType()
Contributor

danomatika commented Sep 6, 2012

The following variables in ofImage should probably be protected/private:

  • width
  • height
  • bpp
  • type

They should be replaced with getters:

  • getWidth()
  • getHeight()
  • getBpp()
  • getType()
@NickHardeman

This comment has been minimized.

Show comment Hide comment
@NickHardeman

NickHardeman Sep 6, 2012

Contributor
Contributor

NickHardeman commented Sep 6, 2012

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor
@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofBaseSoundPlayer.cpp and ofBaseSoundStream.cpp are empty and could be deleted.

Contributor

damian0815 commented Sep 6, 2012

ofBaseSoundPlayer.cpp and ofBaseSoundStream.cpp are empty and could be deleted.

@underdoeg

This comment has been minimized.

Show comment Hide comment
@underdoeg

underdoeg Sep 6, 2012

Contributor

There is a comment refering to openGL smoothing in ofEnableSmoothing. Link makes sense but is in the wrong place. Should be somewhere in ofGLRenderer.

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L560-561

Contributor

underdoeg commented Sep 6, 2012

There is a comment refering to openGL smoothing in ofEnableSmoothing. Link makes sense but is in the wrong place. Should be somewhere in ofGLRenderer.

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofGraphics.cpp#L560-561

@jvcleave
@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofFmodSoundPlayer.cpp is full of getIsPlaying() == true which should be changed to just getIsPlaying().

Contributor

damian0815 commented Sep 6, 2012

ofFmodSoundPlayer.cpp is full of getIsPlaying() == true which should be changed to just getIsPlaying().

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

ofZach Sep 6, 2012

Contributor

ofPixels has alot of code duplication across functions -- for example, crop and cropTo. If it's possible, we should try not to have so much duplication

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofPixels.cpp#L409-491

Contributor

ofZach commented Sep 6, 2012

ofPixels has alot of code duplication across functions -- for example, crop and cropTo. If it's possible, we should try not to have so much duplication

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/graphics/ofPixels.cpp#L409-491

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/sound/ofFmodSoundPlayer.cpp#L232

return (playing != 0 ? true : false)
o_O let's just return playing, yeah?

Contributor

damian0815 commented Sep 6, 2012

https://github.com/openframeworks/openFrameworks/blob/master/libs/openFrameworks/sound/ofFmodSoundPlayer.cpp#L232

return (playing != 0 ? true : false)
o_O let's just return playing, yeah?

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor
@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofPASoundStream::setup and ofRtSoundStream::setup write "Stream over/underflow detected" directly to cout, should be to a log channel.

Contributor

damian0815 commented Sep 6, 2012

ofPASoundStream::setup and ofRtSoundStream::setup write "Stream over/underflow detected" directly to cout, should be to a log channel.

@damian0815

This comment has been minimized.

Show comment Hide comment
@bilderbuchi

This comment has been minimized.

Show comment Hide comment
@bilderbuchi

bilderbuchi Jul 15, 2013

Owner

ofxXmlSettings and ofx3dModelLoader addon structure do not conform to ofxAddonTemplate. Left this way to not break old projects, but should be fixed. cf. PR #2251, commit 870fdcb

Owner

bilderbuchi commented Jul 15, 2013

ofxXmlSettings and ofx3dModelLoader addon structure do not conform to ofxAddonTemplate. Left this way to not break old projects, but should be fixed. cf. PR #2251, commit 870fdcb

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