search

AlloSphere-Research-Group / AlloSystem

AlloSystem is a cross-platform suite of C++ components for building interactive multimedia tools and applications.
http://mat.ucsb.edu/allosphere/software.php
BSD 3-Clause "New" or "Revised" License
65 stars 22 forks source link

Perceptually Linear Color Spaces #31

Closed owengc closed 10 years ago

owengc commented 10 years ago

David pointed out that my pull request should be directed to the devel branch. I just cleaned up a few things and made the transformMatrix variables 'static const' as per Graham's suggestion. Regarding moving the new #include statements into al_Color.cpp, I tried this but I ran into some problems in my test setup when I tried to compile so I'm leaving it as-is for now (this might have just be due to how I've included the files in my test project, though).

owengc commented 10 years ago

I see that my original request on 'master' has been applied to 'devel'. Thank you for doing so. Please consider applying my latest commit on 'devel':

Latest commit has incorporated Graham's suggestion about moving the #include statements for al_Mat and from al_Color.hpp to al_Color.cpp. It also adds a simple example to display color swatches (allocore/examples/graphics/cieColor.cpp). I also found some odd syntax errors in al_Color.cpp which seem to have mysteriously appeared sometime after my last commit because I didn't get any errors from them before.

DrewRWx commented 10 years ago

Err, poor choice of words on my part. I was just closing the pull-request that was incorrectly against master.

LancePutnam commented 10 years ago

Thanks for this. However, it seems that all the tabs have been replaced with spaces creating bogus diffs against the previous code.

LancePutnam commented 10 years ago

I think XYZ is too generic. It will likely be confused with a vector. Maybe CIEXYZ?

owengc commented 10 years ago

That's a fair point about XYZ. I just figured that since it's really more of an intermediate color space most people wouldn't be using it directly, so it only really needed to be unambiguous within al_Color. I'd be happy to update it to CIEXYZ though.

Sorry about the tabs. May I ask what formatting scheme/editor I can use to emulate the existing files now that my formatting has gotten messed up?

LancePutnam commented 10 years ago

I think you just need to ensure your editor does not do any kind of automatic code formatting, namely replacing tabs with spaces. You can use Xcode, TextWrangler, gedit, Notepad++, etc. or any other editor that supports tabs and then just try to match the existing formatting as closely as possible.

matthewjameswright commented 10 years ago

Lots of tools will re-indent your code for you using whatever tab/space regime you specify. If you want to do it by hand you'd enjoy http://en.wikipedia.org/wiki/Whitespace_(programming_language)

owengc commented 10 years ago

I guess that's what I was trying to get at: do you guys use a specific automated tool or tab/space regime that I can just apply to my files? I realize I can do it manually but that's just going to take a lot of time and be more prone to mistakes.

owengc commented 10 years ago

This time I started from fresh copies of the files using a different editor, please let me know if there are still issues with whitespace formatting. I also changed 'XYZ' to 'CIEXYZ'.

LancePutnam commented 10 years ago

The tabs are still replaced by spaces, but this time 4 spaces instead of 2. Just to be clear, the main problem here is that your global formatting changes are creating false diffs. By that I mean diffs that have the same logical code as before, but just different formatting. If you are worried about mistakes, you can use a static code analysis tool (see http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#C.2FC.2B.2B).

We are not using an official code formatting scheme, but here is what I gather: (1) Most of the code is written using tabs not spaces for indenting. (2) We do not indent in a namespace block (presumably as it wastes precious horizontal space in those 80-column editors). (3) We use Unix-style LF line endings rather than CR (Mac) or CRLF (Windows). Be careful that your editor does not change the line endings as that can also produce false diffs.

If we do agree to adopt an automated formatting scheme, then we should do it all at once in a dedicated commit across all the project files.

mantaraya36 commented 10 years ago

You should also be able to set your editor to use tabs instead of spaces somewhere in the preferences.

On Wed, May 14, 2014 at 8:07 AM, Lance Putnam notifications@github.comwrote:

The tabs are still replaced by spaces, but this time 4 spaces instead of

  1. Just to be clear, the main problem here is that your global formatting changes are creating false diffs. By that I mean diffs that have the same logical code as before, but just different formatting. If you are worried about mistakes, you can use a static code analysis tool (see http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis#C.2FC.2B.2B ).

We are not using an official code formatting scheme, but here is what I gather: (1) Most of the code is written using tabs not spaces for indenting. (2) We do not indent in a namespace block (presumably as it wastes precious horizontal space in those 80-column editors). (3) We use Unix-style LF line endings rather than CR (Mac) or CRLF (Windows). Be careful that your editor does not change the line endings as that can also produce false diffs.

If we do agree to adopt an automated formatting scheme, then we should do it all at once in a dedicated commit across all the project files.

— Reply to this email directly or view it on GitHubhttps://github.com/AlloSphere-Research-Group/AlloSystem/pull/31#issuecomment-43092839 .

owengc commented 10 years ago

Thanks for the tips, and thanks for your patience in this matter. I changed my editor's settings and it now appears to be inserting tabs properly

owengc commented 10 years ago

Thanks!