Group: Needs Comments #1552

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

Comments

Projects
None yet
9 participants
@kylemcdonald
Contributor

kylemcdonald commented Sep 6, 2012

Some chunks of openFrameworks need comments to describe their input/output range or the units of their output. This should generally be covered by the documentation, but if a function/method is especially ambiguous then it should have a single line of documentation. If a section has been commented out, it should also be explained why.

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

This comment has been minimized.

Show comment Hide comment
@ofTheo

ofTheo Sep 6, 2012

Contributor

ofPtr has several commented out sections. Why are those commented out? maybe @arturoc

Contributor

ofTheo commented Sep 6, 2012

ofPtr has several commented out sections. Why are those commented out? maybe @arturoc

@arturoc

This comment has been minimized.

Show comment Hide comment
@arturoc

arturoc Sep 6, 2012

Owner

someone tried to add new functionality but it didn't compile on vs2010 so we commented it by now till we find some way to make it work under vs. probably the PR i sent some days ago will fix this and we can remove those comments

Owner

arturoc commented Sep 6, 2012

someone tried to add new functionality but it didn't compile on vs2010 so we commented it by now till we find some way to make it work under vs. probably the PR i sent some days ago will fix this and we can remove those comments

@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

This comment has been minimized.

Show comment Hide comment
@ofZach

ofZach Sep 6, 2012

Contributor
Contributor

ofZach commented Sep 6, 2012

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The ofGetGlInternalFormat, ofGetGlFormatAndType, & ofGetImageTypeFromGLType functions in ofTexture could use some comments.

Contributor

danomatika commented Sep 6, 2012

The ofGetGlInternalFormat, ofGetGlFormatAndType, & ofGetImageTypeFromGLType functions in ofTexture could use some comments.

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofBaseSoundStream needs some comments, eg setDeviceId, setInput and setOutput not clear.

Contributor

damian0815 commented Sep 6, 2012

ofBaseSoundStream needs some comments, eg setDeviceId, setInput and setOutput not clear.

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofBaseVideoPlayer needs comments for ranges and types of arguments for get/setPosition (is this frames or seconds or milliseconds), setSpeed (is this frames per second or a scale relative to normal speed).

Contributor

damian0815 commented Sep 6, 2012

ofBaseVideoPlayer needs comments for ranges and types of arguments for get/setPosition (is this frames or seconds or milliseconds), setSpeed (is this frames per second or a scale relative to normal speed).

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofBaseRenderer pushView/popView: unclear what this does, and caveats when used inside/outside of ofFbo are unclear.

Contributor

damian0815 commented Sep 6, 2012

ofBaseRenderer pushView/popView: unclear what this does, and caveats when used inside/outside of ofFbo are unclear.

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofTrueTypeFont: needs comments about best settings for high-quality output at low point sizes (eg using anti-aliasing and ensuring line height is an integer prevents artefacts on multi-line strings)

Contributor

damian0815 commented Sep 6, 2012

ofTrueTypeFont: needs comments about best settings for high-quality output at low point sizes (eg using anti-aliasing and ensuring line height is an integer prevents artefacts on multi-line strings)

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofBaseVideoPlayer: firstFrame/nextFrame/previousFrame: it's not 100% clear that this is a seek operation, should be commented or called gotoFirstFrame + similar.

Contributor

damian0815 commented Sep 6, 2012

ofBaseVideoPlayer: firstFrame/nextFrame/previousFrame: it's not 100% clear that this is a seek operation, should be commented or called gotoFirstFrame + similar.

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofColor HSB methods need range comments for setting and getting individual values

Contributor

damian0815 commented Sep 6, 2012

ofColor HSB methods need range comments for setting and getting individual values

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofColor:: operator>> needs a comment

Contributor

damian0815 commented Sep 6, 2012

ofColor:: operator>> needs a comment

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofRectangle: scaleIntoMe needs a comment saying how the scaling is performed, eg is the aspect ratio preserved

Contributor

damian0815 commented Sep 6, 2012

ofRectangle: scaleIntoMe needs a comment saying how the scaling is performed, eg is the aspect ratio preserved

@underdoeg

This comment has been minimized.

Show comment Hide comment
@underdoeg

underdoeg Sep 6, 2012

Contributor

@DamianNZ About ofRectangle: scaleIntoMe. there is a already a discussion and PR going on about this #1513

Contributor

underdoeg commented Sep 6, 2012

@DamianNZ About ofRectangle: scaleIntoMe. there is a already a discussion and PR going on about this #1513

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofVec3f rotated(float,float,float) and getRotated/Rad(float,float,float) not clear what this does (is it an Euler rotation?) needs comments.

Contributor

damian0815 commented Sep 6, 2012

ofVec3f rotated(float,float,float) and getRotated/Rad(float,float,float) not clear what this does (is it an Euler rotation?) needs comments.

@NickHardeman

This comment has been minimized.

Show comment Hide comment
@NickHardeman

NickHardeman Sep 6, 2012

Contributor

In ofNode, getPitch(), getHeading() and getRoll() should have a little explanation about each
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/3d/ofNode.h#L51-53

Contributor

NickHardeman commented Sep 6, 2012

In ofNode, getPitch(), getHeading() and getRoll() should have a little explanation about each
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/3d/ofNode.h#L51-53

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The ofTexCompression enums need commenting. Maybe ask Sosolimited ...

Contributor

danomatika commented Sep 6, 2012

The ofTexCompression enums need commenting. Maybe ask Sosolimited ...

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

ofTexture::getCoordFromPoint could be getTextureCoordFromPoint to match other ofTexture functions ... same with getCoordFromPercent.

Contributor

danomatika commented Sep 6, 2012

ofTexture::getCoordFromPoint could be getTextureCoordFromPoint to match other ofTexture functions ... same with getCoordFromPercent.

@bilderbuchi

This comment has been minimized.

Show comment Hide comment
@bilderbuchi

bilderbuchi Sep 6, 2012

Owner

Re: the comments about function parameters: Is there/should there be a specific format we can use so that these comments can be automatically added to the documentation? @arturoc ? I'm not saying doxygen specifically, but some functionally similar thing.

Owner

bilderbuchi commented Sep 6, 2012

Re: the comments about function parameters: Is there/should there be a specific format we can use so that these comments can be automatically added to the documentation? @arturoc ? I'm not saying doxygen specifically, but some functionally similar thing.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The sx/sy & sw/sh variables in the ofTexture::drawSubsection functions should be renamed or the functions commented to better explain their usage aka as an offset coordinate in the larger texture.

Maybe:

  • sx -> offsetX
  • sy -> offsetY
  • sw -> sectionWidth
  • sh -> sectionHeight
Contributor

danomatika commented Sep 6, 2012

The sx/sy & sw/sh variables in the ofTexture::drawSubsection functions should be renamed or the functions commented to better explain their usage aka as an offset coordinate in the larger texture.

Maybe:

  • sx -> offsetX
  • sy -> offsetY
  • sw -> sectionWidth
  • sh -> sectionHeight
@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

@bilderbuchi We're not using doxygen style. Only comment things that are not obvious or need a comment on their range etc ... We're not looking to comment every single variable.

Contributor

danomatika commented Sep 6, 2012

@bilderbuchi We're not using doxygen style. Only comment things that are not obvious or need a comment on their range etc ... We're not looking to comment every single variable.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The ofTextureData texData in ofTexture comment refers to a "smart pointer". What smart pointer ... ?

Contributor

danomatika commented Sep 6, 2012

The ofTextureData texData in ofTexture comment refers to a "smart pointer". What smart pointer ... ?

@arturoc

This comment has been minimized.

Show comment Hide comment
@arturoc

arturoc Sep 6, 2012

Owner

that's there from a try to modify ofTexture to use smartpointers that never worked, it should be removed

Owner

arturoc commented Sep 6, 2012

that's there from a try to modify ofTexture to use smartpointers that never worked, it should be removed

@bilderbuchi

This comment has been minimized.

Show comment Hide comment
@bilderbuchi

bilderbuchi Sep 6, 2012

Owner

@bilderbuchi We're not using doxygen style. Only comment things that are not obvious or need a comment on their range etc ... We're not looking to comment every single variable.

I know. But if it's so non-obvious that we need to add comments in the code, we better add the same comments in the documentation, and my question was if there's an automated way in place to save the duplicated effort and, primarily, make sure both places stay in sync with the actual code when things change later.

Owner

bilderbuchi commented Sep 6, 2012

@bilderbuchi We're not using doxygen style. Only comment things that are not obvious or need a comment on their range etc ... We're not looking to comment every single variable.

I know. But if it's so non-obvious that we need to add comments in the code, we better add the same comments in the documentation, and my question was if there's an automated way in place to save the duplicated effort and, primarily, make sure both places stay in sync with the actual code when things change later.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

There are lots of functions in ofVbo that could use commenting.

For instance, does calling updateVertexData, updateColorData, updateNormalData, or updateTexCoordData resize the Vbo?

Also, what are the variables in setVertexData, setColorData, etc ... what does the "usage" int do?

Contributor

danomatika commented Sep 6, 2012

There are lots of functions in ofVbo that could use commenting.

For instance, does calling updateVertexData, updateColorData, updateNormalData, or updateTexCoordData resize the Vbo?

Also, what are the variables in setVertexData, setColorData, etc ... what does the "usage" int do?

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

In ofCairoRenderer.h, there should be a comment that setting the viewport to an empty rectangle uses the size of the screen.

Contributor

danomatika commented Sep 6, 2012

In ofCairoRenderer.h, there should be a comment that setting the viewport to an empty rectangle uses the size of the screen.

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

ofBeginSaveScreenAsPDF in ofGraphics needs comments. What does b3D do? Also, does setting an empty rectangle for the viewport use the screen size?

Contributor

danomatika commented Sep 6, 2012

ofBeginSaveScreenAsPDF in ofGraphics needs comments. What does b3D do? Also, does setting an empty rectangle for the viewport use the screen size?

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

ofImage::setCompression needs commenting.

Assuming it's the texture compression ... how does it work exactly? Is it only applied when the image is loaded? WHat happens if it's called afterwards?

Contributor

danomatika commented Sep 6, 2012

ofImage::setCompression needs commenting.

Assuming it's the texture compression ... how does it work exactly? Is it only applied when the image is loaded? WHat happens if it's called afterwards?

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

ofImage::reloadTexture needs comments. Does this impact performance? How does it work exactly? What if the image is not using a texture? ...

Contributor

danomatika commented Sep 6, 2012

ofImage::reloadTexture needs comments. Does this impact performance? How does it work exactly? What if the image is not using a texture? ...

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

It's unclear whether switching the ofPath drawing modes mid-path is supposed to work ... Damian says this dosen't seem to work correctly. There should be a comment whether a new subpath is created, the old data is dropped, etc etc when switching modes.

See Damian's dev list email for more info: http://dev.openframeworks.cc/htdig.cgi/of-dev-openframeworks.cc/2012-April/004332.html

Contributor

danomatika commented Sep 6, 2012

It's unclear whether switching the ofPath drawing modes mid-path is supposed to work ... Damian says this dosen't seem to work correctly. There should be a comment whether a new subpath is created, the old data is dropped, etc etc when switching modes.

See Damian's dev list email for more info: http://dev.openframeworks.cc/htdig.cgi/of-dev-openframeworks.cc/2012-April/004332.html

@danomatika

This comment has been minimized.

Show comment Hide comment
@danomatika

danomatika Sep 6, 2012

Contributor

The ofPath api seems to infer that paths can be drawn both filled and with a stroke simultaneously ... does it? I opened #1571 on this. Definitely need comments here ...

Also, what does ofPath::setColor do when there are ofPath::setFillColor() & ofPath::setStrokeColor()?

And more:

What does flagShapeChanged() do exactly? Why would you call it?

What is the default resolution with setCurveResolution()? Same with setArcResolution().

How is setUseShapeColor() related to the global ofStyle color? Is it? Also what "shape color"? Is this the fill, the stroke, what?

What is tessellate() for? It should probably be protected/private ...

Contributor

danomatika commented Sep 6, 2012

The ofPath api seems to infer that paths can be drawn both filled and with a stroke simultaneously ... does it? I opened #1571 on this. Definitely need comments here ...

Also, what does ofPath::setColor do when there are ofPath::setFillColor() & ofPath::setStrokeColor()?

And more:

What does flagShapeChanged() do exactly? Why would you call it?

What is the default resolution with setCurveResolution()? Same with setArcResolution().

How is setUseShapeColor() related to the global ofStyle color? Is it? Also what "shape color"? Is this the fill, the stroke, what?

What is tessellate() for? It should probably be protected/private ...

@NickHardeman

This comment has been minimized.

Show comment Hide comment
@NickHardeman

NickHardeman Sep 6, 2012

Contributor

Some of the functions in ofMath could use some comments, what the input and return ranges are would be useful
Ie. ofLerpDegrees(), ofSignedNoise(), etc.
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMath.cpp#L166-222

Contributor

NickHardeman commented Sep 6, 2012

Some of the functions in ofMath could use some comments, what the input and return ranges are would be useful
Ie. ofLerpDegrees(), ofSignedNoise(), etc.
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/math/ofMath.cpp#L166-222

@NickHardeman

This comment has been minimized.

Show comment Hide comment
@NickHardeman

NickHardeman Sep 6, 2012

Contributor

Not sure of the purpose of these template<> calls in ofUtils, could use some explanation
template <>
string ofToHex( const string& value )
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofUtils.cpp#L304

and
template <> string ofToBinary( const string& value)
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofUtils.cpp#L414

Contributor

NickHardeman commented Sep 6, 2012

Not sure of the purpose of these template<> calls in ofUtils, could use some explanation
template <>
string ofToHex( const string& value )
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofUtils.cpp#L304

and
template <> string ofToBinary( const string& value)
https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/utils/ofUtils.cpp#L414

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofSoundGetSpectrum() -- what is the range of the spectrum values?

Contributor

damian0815 commented Sep 6, 2012

ofSoundGetSpectrum() -- what is the range of the spectrum values?

@underdoeg

This comment has been minimized.

Show comment Hide comment
@underdoeg

underdoeg Sep 6, 2012

Contributor

Soem code for drawing OF_PRIMITIVE_TIRANGLE_FAN is commented out in ofCairoRender. https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofCairoRenderer.cpp#L280

I guess it is not working? Why?

There are also some commented function calls in setStyle like setSmoothingEnabled https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofCairoRenderer.cpp#L171

It is probably not implemented because ofCairoRenderer is mainly intended for vectors that don't need smoothing but when saving as an image, this is a feature we need.

Also setDrawBitmapMode and setCurveResolution and setPolymode is disabled

Contributor

underdoeg commented Sep 6, 2012

Soem code for drawing OF_PRIMITIVE_TIRANGLE_FAN is commented out in ofCairoRender. https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofCairoRenderer.cpp#L280

I guess it is not working? Why?

There are also some commented function calls in setStyle like setSmoothingEnabled https://github.com/openframeworks/openFrameworks/blob/develop/libs/openFrameworks/graphics/ofCairoRenderer.cpp#L171

It is probably not implemented because ofCairoRenderer is mainly intended for vectors that don't need smoothing but when saving as an image, this is a feature we need.

Also setDrawBitmapMode and setCurveResolution and setPolymode is disabled

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofOpenALSoundPlayer::play() error log messages could be more verbose -- ie not printing error codes

Contributor

damian0815 commented Sep 6, 2012

ofOpenALSoundPlayer::play() error log messages could be more verbose -- ie not printing error codes

@damian0815

This comment has been minimized.

Show comment Hide comment
@damian0815

damian0815 Sep 6, 2012

Contributor

ofPASoundStream::stop, start, close, listDevices has error log messages that don't state the function that causes them.

Contributor

damian0815 commented Sep 6, 2012

ofPASoundStream::stop, start, close, listDevices has error log messages that don't state the function that causes them.

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