Ceres nonlinear refactor #248

Alpus · 2019-02-06T14:28:01Z

Add ModelFitter class that encapsulates ceres fitting logic. Using it the user can build needed cost function. Also, it supports a few cameras fitting. Pull request changes ceres example too.

patrikhuber

(removed)

patrikhuber

Hi,

Thanks a lot for the PR! It is quite a large PR. :-) I haven't reviewed all of it yet, but I've left some comments, after a first glance over it.

In general I think it would be good to base the PR off the devel branch, remove all the "zero-changes" and unneeded reformatting changes, this would make the PR quite a bit smaller and easier to grasp what has been changed, so it's more manageable to review. I've put more review comments inline.

Thanks!

patrikhuber · 2019-02-06T18:20:09Z

.gitignore

+.idea/
+cmake-build-*
+
+# Testing data files


I don't think we should put examples/data in .gitignore, as there's already the example images there (and if there are changes to those, we don't want to ignore them). If users have more test data, they should put it somewhere else.

Ok. Will change.

patrikhuber · 2019-02-06T18:20:19Z

.gitignore

+# Mac specific files
+*.DS_Store
+
+# IntelliJ IDEA files


Regarding the IntelliJ files, is it not possible to do out-of-source builds what that IDE? We prefer to recommend that users to out-of-source builds, instead of putting build directories into the source directory and then adding those build directories to .gitignore.
Of course if it's unavoidable IDE project files, we could add it here.

Clion IDE creates a hidden folder .idea/ in the root of the project. Authors of this IDE recommend to add it in .gitignore (https://intellij-support.jetbrains.com/hc/en-us/articles/206544839).

What is about cmake-build-* - it is just build folder and yes, it seems reasonable to remove it from here.

patrikhuber · 2019-02-06T18:22:08Z

examples/fit-model-ceres.cpp

+
+
+#ifndef EOS_CERES_EXAMPLE_USE_PERSPECTIVE
+#define EOS_CERES_EXAMPLE_USE_PERSPECTIVE true


Is it possible to put these defines as variables in main instead? Perhaps as constexpr, so they would be compile-time too? (It should be supported by C++14 I think?)

The idea was to have an option to pass these arguments to compiler. What do you think about it?

patrikhuber · 2019-02-06T18:24:54Z

examples/fit-model-ceres.cpp

+            }
+        };
+
+        struct CliArguments {


I think I'd like to keep the structure of the command-line argument handling like it is in the other examples now. It might in principle be a good idea to refactor those things into methods like you did, but I think particularly the introduction of a new type (CliArguments) and even an exception make the code much less trivial and harder to read for beginners. The example is really meant as a "simple example", and even if the main gets a bit long, I think it's easier to reason about than introducing new types/functions.

It was made to decompose a too big main function. Do you think it is better to put all this code together? Maybe it is better to leave this function, but pass all that it returns as in-out arguments? What do you think? It will avoid new struct creation and will save code decomposition.

patrikhuber · 2019-02-06T18:26:40Z

examples/fit-model-ceres.cpp

+        }
+
+        template<typename LandmarkType>
+        void draw_landmarks(Mat &image, const IndexedLandmarkCollection<LandmarkType> &landmarks,


If you could please use the .clang-format file from the repo for formatting :-) (e.g. in this particular case, the style is to put the ampersand next to the type)

Sorry, just didn't notice .clang-format file.

patrikhuber · 2019-02-06T18:47:05Z

include/eos/fitting/ceres_nonlinear.hpp

+    /**
+     * Block shapes fitting
+     */
+    void block_shapes_fitting() {


With "block" here you mean "set constant"? I think "block" could be confused with the term "parameter block" from Ceres. Perhaps name it set_shape_coefficients_constant()?
(Same applies to the similar functions below)

patrikhuber · 2019-02-06T18:49:32Z

include/eos/fitting/ceres_nonlinear.hpp

+     * * @return std::array with 3 or 4 (for perspective projection) parameters.
+     */
+    auto find_blendshapes(const morphablemodel::Blendshapes* const blendshapes = nullptr) const {
+        if (blendshapes == nullptr) {


I am not really sure I understand the purpose of this method. Why do we need a pointer that can be null, and could we not use MorphableModel::has_separate_expression_model() and eos::cpp17::holds_alternative<...>(...) for the variant?

OK. I will change it.

patrikhuber · 2019-02-06T18:50:49Z

include/eos/fitting/contour_correspondence.hpp

@@ -78,7 +78,7 @@ struct ModelContour

    // We store r/l separately because we currently only fit to the contour facing the camera.
    // Also if we were to fit to the whole contour: Be careful not to just fit to the closest. The
-    // "invisible" ones behind might be closer on an e.g 90� angle. Store CNT for left/right side separately?
+    // "invisible" ones behind might be closer on an e.g 90° angle. Store CNT for left/right side separately?


I'm not sure what's the change on this line - maybe it's a non-ASCII degree sign (°)? It would be great if you could remove all "zero-changes", it would make the PR a bit shorter.

patrikhuber · 2019-02-06T18:51:21Z

include/eos/morphablemodel/MorphableModel.hpp

@@ -177,8 +177,9 @@ class MorphableModel
        core::Mesh mesh;
        if (has_texture_coordinates())
        {
-            mesh = sample_to_mesh(shape, color, shape_model.get_triangle_list(),
-                                  color_model.get_triangle_list(), texture_coordinates);
+            mesh =


I think it would be best to base the PR off the devel branch, then it wouldn't be necessary to cherry-pick this commit.

It is not a problem to base on develop branch but it can't be compiled. So I can't check that everything OK with my work.

Error:

.../include/eos/render/texture_extraction.hpp:609:68: error: no viable constructor or deduction guide for deduction of template arguments of 'tvec4' auto clip_coords = projection_matrix * view_model_matrix * glm::tvec4(vtx.x(), vtx.y(), vtx.z(), 1.0f);

I just don't want to make unrelated to ceres approach changes.

patrikhuber · 2019-02-06T18:52:45Z

include/eos/morphablemodel/PcaModel.hpp

@@ -193,6 +193,14 @@ class PcaModel
        return draw_sample(coeffs_float);
    };

+    template<std::size_t N>
+    Eigen::VectorXf draw_sample(std::array<double, N> coefficients) const


I am not sure the use case is large enough for this to be in the PcaModel file. I would move it for now to the place where it's used (i.e. app or Ceres fitting).

I can move it from here if you want, but there are many such functions in this file. As far as I understand the compiler will remove it from final code if the function will not be used in the program. Do you think it is really needed?

Alpus · 2019-02-08T10:26:45Z

I can push new changes, but I can't build devel "as is". So I can't test my new code :-). Besides, I have some problems with color cost function fitting because I have no texture. I didn't change it before, but to support new eos::core::Image I need to do it.

GCC compiler raise an error when it try to compile braces initializer of IndexLandmark struct. This commit add explicit definition of intializers for this struct.

patrikhuber self-requested a review February 6, 2019 18:13

patrikhuber requested changes Feb 6, 2019

View reviewed changes

Alpus added 2 commits February 7, 2019 12:19

Refactor ceres approach

31897d4

Add IntelliJ IDEA files in .gitignore

d06906e

Alpus added 6 commits February 18, 2019 15:21

Apply clang formatting

19795c4

Remove opencv usage from ceres approach

32e246e

Add PR suggestions

fa1e514

Fix compile error of the GCC with IndexLandmark braces initializer.

b4b9960

GCC compiler raise an error when it try to compile braces initializer of IndexLandmark struct. This commit add explicit definition of intializers for this struct.

Fix a few bugs

2488919

Improve different model support

183fbf3

Alpus force-pushed the ceres_nonlinear_refactor branch from 3fc3fb5 to 183fbf3 Compare February 18, 2019 13:20

Alpus added 3 commits February 18, 2019 16:38

Fix fit-model-multi example

764948a

Remove default ceres options from solve member function

e81b595

Fix forward declaration of get_vertex_colour function

d926432

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ceres nonlinear refactor #248

Ceres nonlinear refactor #248

Alpus commented Feb 6, 2019

patrikhuber left a comment •

edited

Loading

patrikhuber left a comment

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

Alpus Feb 18, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019 •

edited

Loading

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019 •

edited

Loading

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

Alpus Feb 8, 2019

patrikhuber Feb 6, 2019

Alpus Feb 7, 2019

Alpus commented Feb 8, 2019 •

edited

Loading



		#ifndef EOS_CERES_EXAMPLE_USE_PERSPECTIVE
		#define EOS_CERES_EXAMPLE_USE_PERSPECTIVE true

Ceres nonlinear refactor #248

Are you sure you want to change the base?

Ceres nonlinear refactor #248

Conversation

Alpus commented Feb 6, 2019

patrikhuber left a comment • edited Loading

Choose a reason for hiding this comment

patrikhuber left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpus Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpus Feb 7, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Alpus commented Feb 8, 2019 • edited Loading

patrikhuber left a comment •

edited

Loading

Alpus Feb 7, 2019 •

edited

Loading

Alpus Feb 7, 2019 •

edited

Loading

Alpus commented Feb 8, 2019 •

edited

Loading