cgogn / CGoGN_2

n-dimensional Meshes with Combinatorial Maps
https://cgogn.github.io
GNU Lesser General Public License v2.1
20 stars 19 forks source link

Eigen #331

Closed sylvainthery closed 6 years ago

sylvainthery commented 7 years ago

Mais il y a 2 gros points négatifs, pour l'écriture des fonctions:

sylvainthery commented 6 years ago

Gros changements:

untereiner commented 6 years ago

Je ne suis pas très fan du passage en template des attributs juste pour éviter d'expliciter le paramètre template à l'appel des algos. Encore faut-il écrire l'assert qui va bien.

sylvainthery commented 6 years ago

Non seulement ça évite d'expliciter le param template, mais surtout sinon il y a des conflits avec les fonctions geom qui prennent des param non Eigen. Et pour avoir expérimenté les messages d'erreurs sont beaucoup plus clairs !

untereiner commented 6 years ago

Je ne comprend pas. Est-ce que tu as un exemple de fonction ?

sylvainthery commented 6 years ago

Par exemple area, il y a 3 versions:

template <typename VEC3a, typename VEC3b, typename VEC3c>
inline auto area(const Eigen::MatrixBase<VEC3a>& p1, const Eigen::MatrixBase<VEC3b>& p2, const Eigen::MatrixBase<VEC3c>& p3)

template <typename VEC>
inline auto area(const VEC& p1, const VEC& p2, const VEC& p3)

et

template <typename MAP, typename VERTEX_ATTR>
inline ScalarOf<InsideTypeOf<VERTEX_ATTR>> area( MAP& map, const typename MAP::Face f,const VERTEX_ATTR& position)

Quand tu instancies area<VEC3>(...) le compilo considère que tu veux la deuxième version parce quelle a un seul param template

untereiner commented 6 years ago

Pourquoi elle existe la deuxième version ? Pourquoi n'avoir pas que:

template <typename Derived1, typename Derived2, typename Derived3>
inline auto area(const Eigen::MatrixBase<Derived1>& p1, const Eigen::MatrixBase<Derived2>& p2, const Eigen::MatrixBase<Derived3>& p3)

Juste un détail mais le paramètre template qui s'appelle VEC3 pour le type MatrixBase peut être confusant. C'est tout sauf un VEC3.

Donc si je comprend bien pour garder une fonction compatible avec autre chose qu'Eigen on met le type des attributs en paramètre template ?

sylvainthery commented 6 years ago

On ne va pas revenir à chaque fois sur le fait de garder les vec non eigen ! Si c'est des VEC3 ou des operations sur des vec3 (ce qui sémantiquement est un vec3)

Oui a ta dernière question et moi je trouve ça mieux comme ça. L'appel est plus sympa et avec le static_assert, le type checking est le même avec un message d'erreur plus clair