Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Documentation for ofColor. #2779

Merged
merged 12 commits into from
Mar 13, 2014
Merged

Documentation for ofColor. #2779

merged 12 commits into from
Mar 13, 2014

Conversation

bakercp
Copy link
Member

@bakercp bakercp commented Jan 25, 2014

In progress. Please comment.

@bakercp
Copy link
Member Author

bakercp commented Jan 25, 2014

I'm specifically looking for feedback on the use of "channels" vs. "components", etc. It would be nice to land on a consistent language for this as it will also apply to docs for ofImage, ofPixels, etc. Perhaps we could simply adopt the language used in the new packt book / ofBook or @joshuajnoble's work? Others perhaps.

- Use initialization lists.
- Make const argument ordering self-consistent.
- Make method spacing self-consistent.
@bakercp
Copy link
Member Author

bakercp commented Jan 26, 2014

Going with components based on wikipedia.

@bakercp
Copy link
Member Author

bakercp commented Jan 26, 2014

Also, while I'm cleaning this up, is there any particular reason we are passing const references for primitive types for operators in ofColor_? In ofVec* we pass const values for operators (no references) under similar circumstances. Throughout the rest of the codebase it's a mixed bag. For primitive types it doesn't matter from a performance perspective (as far as I understand), but it does relate to documentation, because marking a variable as const in these cases documents intent -- an agreement of sorts -- ... and since I'm writing documentation, I'm just thinking about how we document intent :)

@joshuajnoble
Copy link
Contributor

My thought with channels is that it matches the Photoshop terminology and hence might be a little easier to understand for people,

Obligatory phone disclaimer

On Jan 25, 2014, at 20:47, Christopher Baker notifications@github.com wrote:

Going with components based on wikipedia.


Reply to this email directly or view it on GitHub.

@bakercp
Copy link
Member Author

bakercp commented Jan 26, 2014

Note to self: ask someone (maybe @kylemcdonald) to confirm that getHue() always returns a range of 0-255 for all PixelTypes based on limit() (as it was mentioned in previous comments and as it is hard-coded in getHueAngle()); I'm suspicious.

@bakercp
Copy link
Member Author

bakercp commented Jan 26, 2014

Yeah, I'm pretty sure getHue(), getSaturation() and getBrightness() all return values between 0 and limit(). Thus, I believe that the current getHueAngle() method is incorrect:

template<typename PixelType>
float ofColor_<PixelType>::getHueAngle() const {
    return getHue() * 360. / 255. ;
}

I believe it should be:

template<typename PixelType>
float ofColor_<PixelType>::getHueAngle() const {
    return getHue() * 360. / limit();
}

to return the hue angle in degrees.

@bakercp
Copy link
Member Author

bakercp commented Mar 13, 2014

Not sure what would prevent this from being merged. It was a lot of work.

@admsyn
Copy link
Member

admsyn commented Mar 13, 2014

👍 just testing/reading now and everything looks good to me

Xcode <3's it too

screen

@bakercp
Copy link
Member Author

bakercp commented Mar 13, 2014

For sure! My mini goal was to do the doc and test the style rigorously with doxygen, xcode, etc (which I did).

@bakercp
Copy link
Member Author

bakercp commented Mar 13, 2014

Also @bilderbuchi regarding the ofColor_<PixelType> const & vs. const ofColor_<PixelType>&, since it isn't mentioned in the style guide, I did a quick survey of the core and went with the more frequently used order.

ofZach added a commit that referenced this pull request Mar 13, 2014
@ofZach ofZach merged commit c493bf1 into openframeworks:master Mar 13, 2014
@ofZach
Copy link
Contributor

ofZach commented Mar 13, 2014

just merged this, thanks so much....

@bakercp
Copy link
Member Author

bakercp commented Mar 13, 2014

+1 :)

@bakercp bakercp deleted the doc-ofColor branch March 13, 2014 04:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants