I forgot to answer the test coverage question: no, move() is not covered by any test, even indirectly. The function is not used in other parts of the code currently. B
On Sun, 6 Jan 2019 at 13:44, Bruno Chareyre <[email protected]> wrote: > Hi Janek, > I think that was the right fix, thanks. > Note that yade is not using the periodic triangulation implemented in CGAL > [1] so there is no question on that point afaik. > Bruno > > [1] that's because yade included periodicity years before cgal. I actually > implemented a periodicity based on cgal's non-periodic triangulation. Cgal > devs made periodic regular triangulation available more recently - > triggered by me for a part - but it's still not used in yade. Hopefully it > will be used one day but the refactoring it implies is a bit daunting. > > > > On Fri, 4 Jan 2019 at 19:46, Janek Kozicki <[email protected]> wrote: > >> It appears that this function move_point(…) had long been deprecated. >> I guess that's got something to do with previous modifications where we >> had to use weighted points differently. >> >> I propose to modify this line into calling move(…) instead. >> >> This isn't change like the previous one where the C++ template >> structure was changed and I had to dwell deeply into code to recover >> the original implementation. This is a "small" change of the code >> which I did not write, so I suppose that Bruno I will need you to >> certify this. >> >> Let's see the code in old version: >> /usr/include/CGAL/Regular_triangulation_3.h >> >> #ifndef CGAL_NO_DEPRECATED_CODE >> template < class Gt, class Tds, class Lds > >> typename Delaunay_triangulation_3<Gt,Tds,Default,Lds>::Vertex_handle >> Delaunay_triangulation_3<Gt,Tds,Default,Lds>:: >> move_point(Vertex_handle v, const Point & p) >> { >> CGAL_triangulation_precondition(! is_infinite(v)); >> CGAL_triangulation_expensive_precondition(is_vertex(v)); >> >> // Dummy implementation for a start. >> >> // Remember an incident vertex to restart >> // the point location after the removal. >> Cell_handle c = v->cell(); >> Vertex_handle old_neighbor = c->vertex(c->index(v) == 0 ? 1 : 0); >> CGAL_triangulation_assertion(old_neighbor != v); >> >> remove(v); >> >> if (dimension() <= 0) >> return insert(p); >> return insert(p, old_neighbor->cell()); >> } >> #endif >> >> >> And the code in new version: >> /usr/include/CGAL/Regular_triangulation_3.h >> >> template <class Gt, class Tds, class Lds > >> typename Regular_triangulation_3<Gt,Tds,Lds>::Vertex_handle >> Regular_triangulation_3<Gt,Tds,Lds>:: >> move(Vertex_handle v, const Weighted_point& p) >> { >> CGAL_triangulation_precondition(!is_infinite(v)); >> if(v->point() == p) >> return v; >> >> Self tmp; >> Vertex_remover<Self> remover(tmp); >> Vertex_inserter<Self> inserter(*this); >> return Tr_Base::move(v,p,remover,inserter); >> } >> >> Which calls tr_Base::move(…) from file: >> /usr/include/CGAL/Triangulation_3.h >> >> template < class Gt, class Tds, class Lds > >> template < class VertexRemover, class VertexInserter > >> typename Triangulation_3<Gt,Tds,Lds>::Vertex_handle >> Triangulation_3<Gt,Tds,Lds>:: >> move(Vertex_handle v, const Point& p, >> VertexRemover& remover, VertexInserter& inserter) >> { >> CGAL_assertion(remover.hidden_points_begin() == >> remover.hidden_points_end()); >> CGAL_triangulation_precondition(!is_infinite(v)); >> >> if(v->point() == p) >> return v; >> >> Vertex_handle w = move_if_no_collision(v,p,remover,inserter); >> if(w != v) >> { >> remove(v, remover); >> return w; >> } >> >> return v; >> } >> >> Also let's look at the documentation for move_point: >> >> >> https://doc.cgal.org/latest/Periodic_3_triangulation_3/classCGAL_1_1Periodic__3__Delaunay__triangulation__3.html#aa06932079eb7cf6eee8c5c9998088e34 >> >> It says that it is just a little more optimized regular move. So it >> may be not surprise that those codes are not exactly the same. Maybe >> move_point became obsolete, because they removed the inefficiency >> that was present in older versions. >> >> Note that this function was NOT removed from periodic triangulations. >> This may be important since yade works in periodic too. So perhaps in >> Tesselation.ipp there should be a separate move(…) function for >> periodic cases, to use the old move_point function again when it is >> possible. Bruno? >> >> The compilation passed all tests and checks. But do those tests cover >> the call of _Tesselation<TT>::VertexHandle _Tesselation<TT>::move(…) ? >> >> To encourage discussion I am making a small git commit with one line >> changed ;) >> >> best regards >> Janek Kozicki >> >> _______________________________________________ >> Mailing list: https://launchpad.net/~yade-dev >> Post to : [email protected] >> Unsubscribe : https://launchpad.net/~yade-dev >> More help : https://help.launchpad.net/ListHelp >> >
_______________________________________________ Mailing list: https://launchpad.net/~yade-dev Post to : [email protected] Unsubscribe : https://launchpad.net/~yade-dev More help : https://help.launchpad.net/ListHelp
