search

patrikhuber / eos

A lightweight 3D Morphable Face Model library in modern C++
Apache License 2.0
1.92k stars 598 forks source link

new script to generate edge topology #272

Open kanonet opened 5 years ago

kanonet commented 5 years ago

This is a script to generate the edge_topology.json in python only, without matlab. Generated json does contain the same data like the ones generated with old process, just sorted differently. I think sorting order does not matter in this case, so results are compatible and this script can completely replace the old ones. Please review. Also please elaborate how to give credits to the autors of the original scripts since this is still based on their general idea.

patrikhuber commented 5 years ago

Hi Christoph,

Thanks a lot for the PR, that's a great idea and definitely simplifies the process. I'd like to check the code in a bit more details myself, it'll probably be a while, until around end of August - I'll be travelling/away. But it looks pretty much good to go 👍

From the top of my head, I'd agree that the sorting should not make a difference.

please elaborate how to give credits to the autors of the original scripts

I think if you copy the top of the header from compute_edgestruct.m to the new Python script, that should be completely fine.

Also it would be great if you could convert the tabs to spaces and perhaps run a PEP 8 formatting (though the rest looks pretty consistent). All major Python IDEs should have such an option (I'm using PyCharm) and there are command-line tools for it as well.

Thanks again!

kanonet commented 5 years ago

Hi Patrik,

I cleaned up the code, it should be more readable now. Its PEP8 compliant now (even though I think 79 chars per line are to few...).

Since its all packed in a function, you can call this from your converter scripts to simplify the process even more.

Thanks for creating this project :)

P.S.: I will probably publish some additions to the python binding in next days. Do you want one PR for each function or one PR for every single commit?

patrikhuber commented 5 years ago

Hi Christoph,

Just wanted to let you know that I still have your PR(s) on my todo list. Things are quite busy currently but I should get to it within the next two of weeks. Apologies that it's taking a while.

kanonet commented 5 years ago

Hi @patrikhuber dont want to bother you to much, I just want to know, if you are still working on checking this? Also please not my other PR and those pybind additions that I do in my fork (and would like to PR here, if you want). Best Regards.

patrikhuber commented 5 years ago

Hi Christoph, You're not bothering at all. I apologise that this is taking so long. I have it on my to do list but haven't managed to get to it yet, it has been incredibly busy :/. Please bare with me for a bit longer, I am definitely not forgetting about it.