Adding more fitting python bindings

[omar-h-omar]

Added set_landmark_definitions and altered fit_shape_and_pose to allow fitting without contour.

Note: for the modified fit_shape_and_pose, I allowed users to also pass in pca_coeffs and blendshape_coeffs. This makes it easier to control what is being fitted. I can do the same for the other bindings if you're happy with the approach.

[patrikhuber]

Hi Omar,

I've pushed a slight tweak to set_landmark-definitions() - we don't need to (and don't want to) pass an optional here, we can directly pass in the underlying type, and then assign that to the optional (which now/then contains that value).

About the fit_shape_and_pose(...) overload (for the fitting without contour fitting): How would you feel about leaving the current function as-is (i.e. keep passing the image_points and vertex_indices), but adding an overload for passing landmarks and a landmark mapper, and this overload then computes the mappings and calls the original fit_shape_and_pose(...) function? I think that should work, or would it not? (We can then add Python bindings for both variants.)

Note: for the modified fit_shape_and_pose, I allowed users to also pass in pca_coeffs and blendshape_coeffs. This makes it easier to control what is being fitted. I can do the same for the other bindings if you're happy with the approach.

You mean in the Python bindings, correct? If yes, then yes we could add that for the other fit_shape_and_pose(...) binding too. However I wonder whether this will actually work (in your overload too). The C++ function takes these parameters by reference, i.e. the caller has to have allocated them, and the function then modifies them in-place. Actually I think the way you've done it, it might work, because in the Python bindings, you wrote a custom lambda that takes std::vector<float> (is that the reason you wrote the custom lambda and are not binding it directly via &fit_shape_and_pose? You could bind it directly, you'll need py::overload_cast<...> (which will have a long list of types, see https://pybind11.readthedocs.io/en/stable/classes.html#overloaded-methods for details) - but this may not actually work as I don't know how the reference to vector<float> will behave - if that doesn't work then we will indeed need your approach with the lambda. In the original fit_shape_and_pose binding, I've also created these vectors locally, and then returned them with make_tuple - perhaps it didn't work to bind them directly.)).

Also it would be nice if we could set the default values of these arguments, I think should work. But:

Also I think the default arguments (py::arg("pca_coeffs") = std::vector<float>()) will not work with references anymore, I think they might only work if we take a vector<float> as input in the bindings and return the result with make_tuple like you're doing it now. The thing that I would like to avoid is that a user can't call the function anymore from Python without passing these values, i.e. they should still be callable without these extra parameters.

Well... maybe you could play around with it a bit, but if you get stuck, let me know and I can try it too. Actually, having written all this now, I'm fairly convinced that the way you're doing it now in this PR is probably the best and (almost) only way to do it, so it's probably not worth spending much more time on it. Let me know your thoughts.

[omar-h-omar]

Hi Patrik,

I saw your commits. I was experimenting with the parameters and it looks like I forgot to make the function parameter mandatory.

For the fit_shape_and_pose, I agree we should leave the old function in its current form. It would also make sure that anyone relying on that function will still be able to use it. If it's okay, I am also going to leave the old overload for that function so users can choose whether to pass in the pca_coeffs and blendshape_coeffs or not. I'll then make an overload just for the landmark mapper version.

As for my note, yes I was referring to the python bindings. I did some experimenting when I initially made that change. From what I understand, by default pybind doesn't pass the Python params by reference. Instead it instantiates a C++ version of those parameter. Any updates to those parameters are then not passed back to python. Here's a stackflow discussion that I found that explains it a bit better: https://stackoverflow.com/questions/72885134/pybind11-pointer-reference. I am not very knowledgeable with C++ so feel free to correct if I am wrong but I think my solution gets around this issue.

[omar-h-omar]

Hi Patrik,

I was checking out the set_landmark_definitions locally and noticed an edge case.

In the fitting functions, we check if the landmark definitions are not None and if so we use them. However, the set_landmark_definitions function in this PR doesn't allow optional parameters like None. As a result, a user can bring in a model that doesn't have any landmark definitions, set landmark definitions and save the model as normal. However, if they want to go back and remove those definitions they can't. They can only replace them with new ones.

This seems quite limiting. I don't think it is what we want right?

[patrikhuber]

Hi Omar,

This seems quite limiting. I don't think it is what we want right?

You're completely right, very well spotted. Actually in your original PR you were passing an optional<...> and I removed that in a5deb44 as I thought it's not needed, but I haven't thought of the edge case that you spotted. I think changing the function back to taking an eos::cpp17::optional<...> should work?

I'll have a more close look at your other message above from 14 Apr but I've had a quick look back then and I think I pretty much agree with your assessment!

[omar-h-omar]

Hi Omar,

This seems quite limiting. I don't think it is what we want right?

You're completely right, very well spotted. Actually in your original PR you were passing an optional<...> and I removed that in a5deb44 as I thought it's not needed, but I haven't thought of the edge case that you spotted. I think changing the function back to taking an eos::cpp17::optional<...> should work?

I'll have a more close look at your other message above from 14 Apr but I've had a quick look back then and I think I pretty much agree with your assessment!

Great! Yeah adding eos::cpp17::optional<...> does work. I'll update the PR now :))

[patrikhuber]

Hi @omar-h-omar,

I just had a final look at this PR. Could I just confirm with you - above we said we want to leave the current fit_shape_and_pose overloads, and add an overload that takes the landmarks & LandmarkMapper, and calls the existing function. I think as far as I can see, this change is not in the PR yet, and the current PR changes the old functions. Do you concur with that?

Other than that, it looks all good to me and happy to merge it.

If you're busy, let me know, and I can make the change on the weekend as well.

Best wishes,
Patrik

[omar-h-omar]

Hi @patrikhuber

Sorry about that. Looks like I forgot to add new overloads instead of modifying the old ones. I've added them now and updated the params in the overload briefs. It should be ready to merge now :)

All the best
Omar

[patrikhuber]

Looks great now @omar-h-omar, thank you very much!