Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12096069:
New Matrix3F class added in gfx. (Closed)
|
Created:
7 years, 10 months ago by motek. Modified:
7 years, 10 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded Matrix3F class to gfx.
Will need it to implement color decomposition for thumbnails.
BUG=155269
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=180427
Patch Set 1 #
Total comments: 40
Patch Set 2 : Addressed reviewer's remarks/ #Patch Set 3 : Addressed reviewer's remarks (re-do). #
Total comments: 10
Patch Set 4 : Review fixes (2). #Patch Set 5 : A typo fix. #
Total comments: 34
Patch Set 6 : Addressing review comments. #Patch Set 7 : Completed renaming + a float PI. #
Total comments: 14
Patch Set 8 : Remaining nits. #
Total comments: 6
Patch Set 9 : Another round + precision fix-up. #
Messages
Total messages: 27 (0 generated)
Here are some initial comments. I'm also curious to see the code that will be using this, to see how the API is used. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode15 ui/gfx/matrix3_f.cc:15: enum { // This is only to make accessing indices self-explanatory. Nit: Move the comment above the enum and name the enum. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode29 ui/gfx/matrix3_f.cc:29: double Determinant3x3(T data[9]) { Is there a reason this implementation can't be inlined into Matrix3F::Determinant()? https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode38 ui/gfx/matrix3_f.cc:38: } Nit: Add " // namespace" at the end of the }. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode117 ui/gfx/matrix3_f.cc:117: // The matrix must be symmetric. Document this in the comment in the header. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode126 ui/gfx/matrix3_f.cc:126: ScalarType eigenvalues[3]; Does it make sense to use a Vector3F here? https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode128 ui/gfx/matrix3_f.cc:128: data_[kM02] * data_[kM02] + Nit: Align with data[kM01]. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode181 ui/gfx/matrix3_f.cc:181: Nit: Remove one of the empty lines. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode186 ui/gfx/matrix3_f.cc:186: for (int i = 0; i < 3; ++i) { Nit: No need for {'s when the body is only one line. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode254 ui/gfx/matrix3_f.cc:254: matrix.set(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0); Add a version of set() that has a single parameter that sets all entries to that value and use it here and in Ones(). https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode271 ui/gfx/matrix3_f.cc:271: Nit: Remove one of the blank lines. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode19 ui/gfx/matrix3_f.h:19: ~Matrix3F() { } Move the body to the implementation. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode21 ui/gfx/matrix3_f.h:21: bool operator==(const Matrix3F& rhs) const; Other types in ui/gfx define this outside of the class, please do the same for consistency. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode36 ui/gfx/matrix3_f.h:36: Matrix3F* Inverse() const; Return by value. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode38 ui/gfx/matrix3_f.h:38: ScalarType Trace() const; Please add comments for most of these methods. Probably set() and get() can stay without comments. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode41 ui/gfx/matrix3_f.h:41: VectorType* PositiveDefiniteEigensolver(Matrix3F* eigenvectors) const; - Choose a better name, e.g. one that has a verb in it. e.g. Compute... - Document the return value and parameter and what happens e.g. when someone passes in NULL, since the implementation cares about it. - Return by value. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode50 ui/gfx/matrix3_f.h:50: Matrix3F() { } // Uninitialized default. Move the body to the implementation. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode53 ui/gfx/matrix3_f.h:53: } Nit: Add " // namespace gfx" https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:27: for (int i=0; i < 3; ++i) { Nit: "i=0" -> "i = 0". https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:28: for (int j = 0; j < 3; ++j) { Nit: No need for {'s. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:95: // This block tests the trivial case of eigenvalues of the identity Move these into separate testcases, e.g you can name them Eigenvectors_Identity, etc.
Added you as the reviewer on https://chromiumcodereview.appspot.com/12091093/ where this will be used. The whole point is that I'll need to call SolveEigenproblem on color covariance matrix there and use it to get an intensity image. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode15 ui/gfx/matrix3_f.cc:15: enum { // This is only to make accessing indices self-explanatory. On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: Move the comment above the enum and name the enum. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode29 ui/gfx/matrix3_f.cc:29: double Determinant3x3(T data[9]) { On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Is there a reason this implementation can't be inlined into > Matrix3F::Determinant()? Need higher precision for computing the inverse. Note the use of (double) here rather than Matrix3F::ScalarType (whatever that may be). https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode38 ui/gfx/matrix3_f.cc:38: } On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: Add " // namespace" at the end of the }. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode117 ui/gfx/matrix3_f.cc:117: // The matrix must be symmetric. On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Document this in the comment in the header. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode117 ui/gfx/matrix3_f.cc:117: // The matrix must be symmetric. On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Document this in the comment in the header. Positive-definite implies symmetric, I believe. Either way, I did add an explicit comment about it. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode126 ui/gfx/matrix3_f.cc:126: ScalarType eigenvalues[3]; On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Does it make sense to use a Vector3F here? It doesn't. I need to swap values and access them by index, stuff that Vector3F doesn't have or makes way to verbose. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode128 ui/gfx/matrix3_f.cc:128: data_[kM02] * data_[kM02] + On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: Align with data[kM01]. Is that how we do it? I mean: emacs even doesn't let me do it. Anyway, done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode181 ui/gfx/matrix3_f.cc:181: On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: Remove one of the empty lines. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode186 ui/gfx/matrix3_f.cc:186: for (int i = 0; i < 3; ++i) { On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: No need for {'s when the body is only one line. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.cc#newcode254 ui/gfx/matrix3_f.cc:254: matrix.set(0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0, 0.0); On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Add a version of set() that has a single parameter that sets all entries to that > value and use it here and in Ones(). Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode19 ui/gfx/matrix3_f.h:19: ~Matrix3F() { } On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Move the body to the implementation. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode21 ui/gfx/matrix3_f.h:21: bool operator==(const Matrix3F& rhs) const; On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Other types in ui/gfx define this outside of the class, please do the same for > consistency. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode36 ui/gfx/matrix3_f.h:36: Matrix3F* Inverse() const; On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Return by value. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode38 ui/gfx/matrix3_f.h:38: ScalarType Trace() const; On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Please add comments for most of these methods. Probably set() and get() can stay > without comments. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode41 ui/gfx/matrix3_f.h:41: VectorType* PositiveDefiniteEigensolver(Matrix3F* eigenvectors) const; On 2013/01/31 15:54:59, Alexei Svitkine wrote: > - Choose a better name, e.g. one that has a verb in it. e.g. Compute... > > - Document the return value and parameter and what happens e.g. when someone > passes in NULL, since the implementation cares about it. > > - Return by value. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode50 ui/gfx/matrix3_f.h:50: Matrix3F() { } // Uninitialized default. On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Move the body to the implementation. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_f.h#newcode53 ui/gfx/matrix3_f.h:53: } On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: Add " // namespace gfx" Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:27: for (int i=0; i < 3; ++i) { On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: "i=0" -> "i = 0". Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:28: for (int j = 0; j < 3; ++j) { On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Nit: No need for {'s. Done. https://codereview.chromium.org/12096069/diff/1/ui/gfx/matrix3_unittest.cc#ne... ui/gfx/matrix3_unittest.cc:95: // This block tests the trivial case of eigenvalues of the identity On 2013/01/31 15:54:59, Alexei Svitkine wrote: > Move these into separate testcases, e.g you can name them Eigenvectors_Identity, > etc. Done.
Looking good, just a few more things. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode30 ui/gfx/matrix3_f.cc:30: double Determinant3x3(T data[kMEnd]) { Add a comment explaining the purpose (same as you reply to me that you need a higher precision version.) https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode31 ui/gfx/matrix3_f.cc:31: return (double)data[kM00] * ((double)data[kM11] * data[kM22] - Use C++ casts, rather than C-style casts. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode75 ui/gfx/matrix3_f.cc:75: return data_[i * 3 + j]; Make a function in the anon namespace that gives you the index, and stick the dchecks there. Then, use that whenever you index into the array via i,j. e.g. int GetElementIndex(int i, int j) { DCHECK(i >= 0 && i < 3); DCHECK(j >= 0 && j < 3); return i * 3 + j; } https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcod... ui/gfx/matrix3_f.cc:117: double det = Determinant3x3(data_); Nit: det -> determinant https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.h#newcode18 ui/gfx/matrix3_f.h:18: Matrix3F(const VectorType& a, const VectorType& bt); This is the outer-product right? Consider making this a static function returning a Matrix3F with a name that makes that clear, or at the very least add a comment here.
https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode30 ui/gfx/matrix3_f.cc:30: double Determinant3x3(T data[kMEnd]) { On 2013/01/31 20:02:38, Alexei Svitkine wrote: > Add a comment explaining the purpose (same as you reply to me that you need a > higher precision version.) Done. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode31 ui/gfx/matrix3_f.cc:31: return (double)data[kM00] * ((double)data[kM11] * data[kM22] - On 2013/01/31 20:02:38, Alexei Svitkine wrote: > Use C++ casts, rather than C-style casts. Done. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcode75 ui/gfx/matrix3_f.cc:75: return data_[i * 3 + j]; On 2013/01/31 20:02:38, Alexei Svitkine wrote: > Make a function in the anon namespace that gives you the index, and stick the > dchecks there. Then, use that whenever you index into the array via i,j. > > e.g. > > int GetElementIndex(int i, int j) { > DCHECK(i >= 0 && i < 3); > DCHECK(j >= 0 && j < 3); > return i * 3 + j; > } > Done. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.cc#newcod... ui/gfx/matrix3_f.cc:117: double det = Determinant3x3(data_); On 2013/01/31 20:02:38, Alexei Svitkine wrote: > Nit: det -> determinant Done. https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/6002/ui/gfx/matrix3_f.h#newcode18 ui/gfx/matrix3_f.h:18: Matrix3F(const VectorType& a, const VectorType& bt); On 2013/01/31 20:02:38, Alexei Svitkine wrote: > This is the outer-product right? > > Consider making this a static function returning a Matrix3F with a name that > makes that clear, or at the very least add a comment here. Done.
LGTM with a couple remaining comments. Adding danakj@ as a reviewer too, to make sure this looks fine to her too. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:112: return VectorType(data_[i], data_[3 + i], data_[6 + i]); Use MatrixToArrayCoords() here. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:117: data_[i] = c.x(); Use MatrixToArrayCoords() here.
Fixed these + added sky@ to reviewer's list for owner's review of changes to ui/*.gy* files. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:112: return VectorType(data_[i], data_[3 + i], data_[6 + i]); On 2013/01/31 22:17:18, Alexei Svitkine wrote: > Use MatrixToArrayCoords() here. Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:117: data_[i] = c.x(); On 2013/01/31 22:17:18, Alexei Svitkine wrote: > Use MatrixToArrayCoords() here. Done.
Small setter/getter methods in the .cpp will have their runtime dominated by the effort to make a function call. They should live in the header instead. I don't think I see the benefit to ScalarType and VectorType. The class name already denotes that these are floats. Why not just use "float" and "Vector3dF" throughout? https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:21: kM00, enum names should be CAPS without the k prefix. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:50: int MatrixToArrayCoords(int i, int j) { make this a private member function in the header so you can use it from get() and set() https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:84: return data_[MatrixToArrayCoords(i, j)]; move this to the header https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:88: data_[MatrixToArrayCoords(i, j)] = v; move this to the header https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:91: void Matrix3F::set(ScalarType v) { set the whole matrix to a single value? is this really something that will be done commonly? i've never seen this on a matrix before. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:110: Matrix3F::VectorType Matrix3F::GetColumn(int i) const { move to header. get_column() https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:115: void Matrix3F::SetColumn(int i, const VectorType& c) { move to header https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:142: return ScalarType(Determinant3x3(data_)); static cast<ScalarType>() https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:145: Matrix3F::ScalarType Matrix3F::Trace() const { move to header https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:273: matrix.set(0.0); if this is the only use of set(float), i'd prefer set(0,0,0,0,0,0,0,0,0) here and in Ones() https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:287: matrix.set(1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0); use 1.f or 1.0f for float literals. same for 0.0, throughout this file. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.h#newcode24 ui/gfx/matrix3_f.h:24: bool IsNear(const Matrix3F& rhs, ScalarType precision) const; nit: primarily If this is just for tests, can you move it to the unittest file please?
https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:12: using gfx::Matrix3F; please put this file in namespace gfx { namespace { // tests here.. } } Then you don't need this using.
https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:12: using gfx::Matrix3F; On 2013/01/31 22:40:55, danakj wrote: > please put this file in > > namespace gfx { > namespace { You don't need the inner namespace. Just "namespace gfx {" should be fine.
https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:12: using gfx::Matrix3F; On 2013/01/31 22:45:05, Alexei Svitkine wrote: > On 2013/01/31 22:40:55, danakj wrote: > > please put this file in > > > > namespace gfx { > > namespace { > > You don't need the inner namespace. Just "namespace gfx {" should be fine. It's true, but this is the general practice used elsewhere AFAIK.
On 2013/01/31 22:46:41, danakj wrote: > https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.cc > File ui/gfx/matrix3_unittest.cc (right): > > https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.c... > ui/gfx/matrix3_unittest.cc:12: using gfx::Matrix3F; > On 2013/01/31 22:45:05, Alexei Svitkine wrote: > > On 2013/01/31 22:40:55, danakj wrote: > > > please put this file in > > > > > > namespace gfx { > > > namespace { > > > > You don't need the inner namespace. Just "namespace gfx {" should be fine. > > It's true, but this is the general practice used elsewhere AFAIK. FWIW, I've not seen it used elsewhere. In gfx, at least a dozen or more files I'm familiar with don't use it...
On Thu, Jan 31, 2013 at 5:49 PM, <asvitkine@chromium.org> wrote: > FWIW, I've not seen it used elsewhere. > > In gfx, at least a dozen or more files I'm familiar with don't use it... > Hm, yeh. Maybe that's more of a cc/-ism at the moment. Some of the gfx test files do. Anyway, not a big deal either way I think.
Done (except k-constants, where I would like a clarification). In particular, I got rid of VectorType and ScalarType. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:21: kM00, On 2013/01/31 22:38:30, danakj wrote: > enum names should be CAPS without the k prefix. Is this a recent change to style guide? The only other enum declared in a cc file in this module (in skbitmap_operation.cc) uses the k-camel convention. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:50: int MatrixToArrayCoords(int i, int j) { On 2013/01/31 22:38:30, danakj wrote: > make this a private member function in the header so you can use it from get() > and set() Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:84: return data_[MatrixToArrayCoords(i, j)]; On 2013/01/31 22:38:30, danakj wrote: > move this to the header Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:88: data_[MatrixToArrayCoords(i, j)] = v; On 2013/01/31 22:38:30, danakj wrote: > move this to the header Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:91: void Matrix3F::set(ScalarType v) { On 2013/01/31 22:38:30, danakj wrote: > set the whole matrix to a single value? is this really something that will be > done commonly? i've never seen this on a matrix before. Honestly? I had no use for it either. It has been requested by the other asvitkine@ in the review (used from Zeros() and Ones()). Should I get rid of it? Sure, its gone. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:110: Matrix3F::VectorType Matrix3F::GetColumn(int i) const { On 2013/01/31 22:38:30, danakj wrote: > move to header. get_column() Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:115: void Matrix3F::SetColumn(int i, const VectorType& c) { On 2013/01/31 22:38:30, danakj wrote: > move to header Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:142: return ScalarType(Determinant3x3(data_)); On 2013/01/31 22:38:30, danakj wrote: > static cast<ScalarType>() Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:145: Matrix3F::ScalarType Matrix3F::Trace() const { On 2013/01/31 22:38:30, danakj wrote: > move to header Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:273: matrix.set(0.0); On 2013/01/31 22:38:30, danakj wrote: > if this is the only use of set(float), i'd prefer set(0,0,0,0,0,0,0,0,0) here > and in Ones() Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:287: matrix.set(1.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 1.0); On 2013/01/31 22:38:30, danakj wrote: > use 1.f or 1.0f for float literals. same for 0.0, throughout this file. Done. https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.h#newcode24 ui/gfx/matrix3_f.h:24: bool IsNear(const Matrix3F& rhs, ScalarType precision) const; On 2013/01/31 22:38:30, danakj wrote: > nit: primarily > > If this is just for tests, can you move it to the unittest file please? On the second thought: I may actually need it in practice soon enough (will not want to call eigensolver on covariance matrix if it is close to 0). https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:12: using gfx::Matrix3F; On 2013/01/31 22:40:55, danakj wrote: > please put this file in > > namespace gfx { > namespace { > > // tests here.. > > } > } > > > Then you don't need this using. Done.
https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:21: kM00, On 2013/01/31 23:29:56, motek. wrote: > On 2013/01/31 22:38:30, danakj wrote: > > enum names should be CAPS without the k prefix. > Is this a recent change to style guide? The only other enum declared in a cc > file in this module (in skbitmap_operation.cc) uses the k-camel convention. There was a recent thread on chromium-dev a month or so back about it. There is a bit of inconsistency in existing code, but the consensus which is in the style guide now was: "Though the Google C++ Style Guide now says to use kConstantNaming for enums, Chromium was written using MACRO_STYLE naming. Continue to use this style for consistency."
https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:91: void Matrix3F::set(ScalarType v) { On 2013/01/31 23:29:56, motek. wrote: > On 2013/01/31 22:38:30, danakj wrote: > > set the whole matrix to a single value? is this really something that will be > > done commonly? i've never seen this on a matrix before. > Honestly? I had no use for it either. It has been requested by the other > asvitkine@ in the review (used from Zeros() and Ones()). > Should I get rid of it? Sure, its gone. Deferring to Dana here, I just suggested it because it's more concise from the call site than specifying 9 constants. But if that's an uncommon use, then kill it.
On 2013/01/31 23:32:56, danakj wrote: > https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc > File ui/gfx/matrix3_f.cc (right): > > https://codereview.chromium.org/12096069/diff/29001/ui/gfx/matrix3_f.cc#newco... > ui/gfx/matrix3_f.cc:21: kM00, > On 2013/01/31 23:29:56, motek. wrote: > > On 2013/01/31 22:38:30, danakj wrote: > > > enum names should be CAPS without the k prefix. > > Is this a recent change to style guide? The only other enum declared in a cc > > file in this module (in skbitmap_operation.cc) uses the k-camel convention. > > There was a recent thread on chromium-dev a month or so back about it. There is > a bit of inconsistency in existing code, but the consensus which is in the style > guide now was: > > "Though the Google C++ Style Guide now says to use kConstantNaming for enums, > Chromium was written using MACRO_STYLE naming. Continue to use this style for > consistency." Thanks. Done as suggested.
LGTM+nits. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:77: float m20, float m21, float m22) { oh, missed this. i think this belongs in the header as well as it's just assignments. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:139: p = sqrt(p / 6); nit: std::sqrt https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:142: Matrix3F matrix_B(*this); nit: matrix_b https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:209: e2.set_x(-e2.x()); can use e2 = -e2; ? https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:215: e3.set_x(-e3.x()); e3 = -e3; ? https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.h#newcode76 ui/gfx/matrix3_f.h:76: static Matrix3F Zeros(); nit: Move these up to the top of the class where the constructor lives. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. 2013
All done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:77: float m20, float m21, float m22) { On 2013/02/01 00:02:34, danakj wrote: > oh, missed this. i think this belongs in the header as well as it's just > assignments. Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:139: p = sqrt(p / 6); On 2013/02/01 00:02:34, danakj wrote: > nit: std::sqrt Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:142: Matrix3F matrix_B(*this); On 2013/02/01 00:02:34, danakj wrote: > nit: matrix_b Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:209: e2.set_x(-e2.x()); On 2013/02/01 00:02:34, danakj wrote: > can use e2 = -e2; ? Ah, didn't notice the op. Cool. Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:215: e3.set_x(-e3.x()); On 2013/02/01 00:02:34, danakj wrote: > e3 = -e3; ? Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_f.h#newcode76 ui/gfx/matrix3_f.h:76: static Matrix3F Zeros(); On 2013/02/01 00:02:34, danakj wrote: > nit: Move these up to the top of the class where the constructor lives. Done. https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_unittest.cc File ui/gfx/matrix3_unittest.cc (right): https://codereview.chromium.org/12096069/diff/14002/ui/gfx/matrix3_unittest.c... ui/gfx/matrix3_unittest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/02/01 00:02:34, danakj wrote: > 2013 Done.
https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:55: Matrix3F::Matrix3F(const Matrix3F& rhs) { Is there a reason to do this rather than letting the compiler generate it for you? https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h#newcode92 ui/gfx/matrix3_f.h:92: float data_[9]; fields after functions. https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h#newcode94 ui/gfx/matrix3_f.h:94: Matrix3F(); // Uninitialized default. Seems painful not to have this and let it return the identity.
I'm just curious about the memcpy. Not having a public empty constructor seems painful, but then I don't understand all the use cases.
Responded to question, fixed nits. Thanks for help, BTW. I also realized that in one place in eigensolver computation should be done in double, so I fixed that, too. Now, given that I got LGTM twice on this CL, I would like to try and land it. Unless there are still concerns, that is... https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.cc File ui/gfx/matrix3_f.cc (right): https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.cc#newco... ui/gfx/matrix3_f.cc:55: Matrix3F::Matrix3F(const Matrix3F& rhs) { On 2013/02/01 00:25:30, sky wrote: > Is there a reason to do this rather than letting the compiler generate it for > you? The only reason is that I believed doing that leaving anything implicit was frowned upon. So, when grooming the CL for review I added that. I'll remove it gladly as you seem to suggest. So: Done. https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h File ui/gfx/matrix3_f.h (right): https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h#newcode92 ui/gfx/matrix3_f.h:92: float data_[9]; On 2013/02/01 00:25:30, sky wrote: > fields after functions. Done. https://codereview.chromium.org/12096069/diff/36004/ui/gfx/matrix3_f.h#newcode94 ui/gfx/matrix3_f.h:94: Matrix3F(); // Uninitialized default. On 2013/02/01 00:25:30, sky wrote: > Seems painful not to have this and let it return the identity. I don't think making the constructor set identity implicitly is not that useful. They went through a similar exercise in Skia and ended up deprecating the default constructor. Instead, I have a set of 'creator' static functions that address reasonable possibilities. I am sure the compiler will make a decent work of it.
LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/12096069/36005
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/12096069/36005
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/motek@chromium.org/12096069/36005
Message was sent while issue was closed.
Change committed as 180427 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
