yade-dev team mailing list archive

[Bruno Chareyre <bruno.chareyre@xxxxxxxxxxxxxxx>]

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 <bruno.chareyre@xxxxxxxxxxxxxxx>
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 <janek_listy@xxxxx> 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     : yade-dev@xxxxxxxxxxxxxxxxxxx
>> Unsubscribe : https://launchpad.net/~yade-dev
>> More help   : https://help.launchpad.net/ListHelp
>>
>