Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improvement backports from CDT_3 branch (Follow-up to PR #8170) #8273

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Improvement backports from CDT_3 branch (Follow-up to PR #8170) #8273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c10dcf7
allow to call save_binary_file on non-C3t3 type (like a CDT_3)
lrineau Jun 10, 2024
93fd966
use boost::unordered_flat_map to optimize Polyline_constraint_hierarc…
lrineau Apr 15, 2024
82b5359
Mesh_2: sort the sequence tr.subconstraints_begin(), tr.subconstraint…
lrineau Jun 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 19 additions & 5 deletions Mesh_2/include/CGAL/Mesh_2/Refine_edges.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,12 +346,26 @@ class Refine_edges_base :
{
// with constraint hierarchy

for(typename Tr::Subconstraint_iterator it = tr.subconstraints_begin();
it != tr.subconstraints_end(); ++it)
{
const Vertex_handle& v1 = it->first.first;
const Vertex_handle& v2 = it->first.second;
// create a vector of pairs of vertex handles, from the subconstraints
// and sort it to ensure the determinism
std::vector<std::array<Vertex_handle, 2>> subconstraints_vector(tr.number_of_subconstraints());
std::transform(tr.subconstraints_begin(), tr.subconstraints_end(), subconstraints_vector.begin(),
[](const auto& sc) {
return std::array<Vertex_handle, 2>{sc.first.first, sc.first.second};
});

auto comp_vh = [&] (Vertex_handle va, Vertex_handle vb) {
return tr.compare_xy(va->point(), vb->point()) == SMALLER;
};
auto comp_pair_vh = [&] (const auto& e1, const auto& e2) {
return comp_vh(e1[0], e2[0]) ||
(!comp_vh(e2[0], e1[0]) && comp_vh(e1[1], e2[1]));
};

std::sort(subconstraints_vector.begin(), subconstraints_vector.end(), comp_pair_vh);

for(const auto& [v1, v2] : subconstraints_vector)
{
if(!is_locally_conform(tr, v1, v2) ){
add_constrained_edge_to_be_conformed(v1, v2);
}
Expand Down
3 changes: 1 addition & 2 deletions SMDS_3/include/CGAL/IO/File_binary_mesh_3.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,14 +46,13 @@ save_binary_file(std::ostream& os,
, bool binary = true)
#endif
{
typedef typename C3T3::Triangulation::Geom_traits::FT FT;
if(binary) os << "binary ";
os << "CGAL c3t3 " << CGAL::Get_io_signature<C3T3>()() << "\n";
if(binary) {
CGAL::IO::set_binary_mode(os);
} else {
CGAL::IO::set_ascii_mode(os);
os.precision(std::numeric_limits<FT>::digits10+2);
os.precision(std::numeric_limits<double>::max_digits10);
}
return !!(os << c3t3);
// call operator!() twice, because operator bool() is C++11
Expand Down
38 changes: 16 additions & 22 deletions Triangulation_2/include/CGAL/Triangulation_2/internal/Polyline_constraint_hierarchy_2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,15 @@
#include <map>
#include <set>
#include <list>

#include <boost/version.hpp>
#if BOOST_VERSION >= 108100
# include <boost/unordered/unordered_flat_map.hpp>
# define CGAL_USE_BOOST_UNORDERED 1
#else // BOOST before 1.81.0
# include <unordered_map>
#endif

#include <CGAL/Skiplist.h>
#include <CGAL/assertions.h>

Expand Down Expand Up @@ -134,24 +143,6 @@ class Polyline_constraint_hierarchy_2
}
};

class Pair_compare {
Compare comp;

public:
Pair_compare(const Compare& comp) : comp(comp) {}

bool operator()(const Edge& e1, const Edge& e2) const {
if(comp(e1.first, e2.first)) {
return true;
} else if((! comp(e2.first, e1.first)) && // !less(e1,e2) && !less(e2,e1) == equal
comp(e1.second, e2.second)) {
return true;
} else {
return false;
}
}
};

class Context {
friend class Polyline_constraint_hierarchy_2<T,Compare,Point>;
private:
Expand All @@ -171,8 +162,11 @@ class Polyline_constraint_hierarchy_2
typedef typename Context_list::iterator Context_iterator;

typedef std::set<Constraint_id> Constraint_set;
typedef std::map<Edge, Context_list*,
Pair_compare> Sc_to_c_map;
#if CGAL_USE_BOOST_UNORDERED
typedef boost::unordered_flat_map<Edge, Context_list*, boost::hash<Edge>> Sc_to_c_map;
#else
typedef std::unordered_map<Edge, Context_list*, boost::hash<Edge>> Sc_to_c_map;
#endif
Comment on lines +165 to +169
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Order of the sequence of values from the range CGAL::Constrained_triangulation_plus_2<Tr>::subconstraints()

@janetournois @afabri

  • The patch 93fd966 makes the range CGAL::Constrained_triangulation_plus_2<Tr>::subconstraints() non-deterministic: instead of a std::map with a custom compare functor, now the sub-constraints are in an unordered map, for efficiency reasons. Note that the range was never documented as sorted or deterministic.
  • As my patch had broken a test from Mesh_2 (the test test_meshing), I had to fix that non-determinism. That was complicated to fix directly in Constrained_triangulation_plus_2 or Constrained_triangulation_plus_2. Instead, I have chosen to fix the only place in CGAL where the order of that range mattered, in Mesh_2/include/CGAL/Mesh_2/Refine_edges.h. See the commit 82b5359.

Is that correct? That is a sort of breaking change, but the ordering of Ctp_2::constraints() was never documented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jane (IRL) suggested that:

  • it is documented that there is a breaking change
  • and add a Tag to Constrained_triangulation_plus_2 to allow users to fallback to the previous behavior.

This comment was marked as off-topic.

Sign in to view

typedef typename Constraint_set::iterator C_iterator;
typedef typename Sc_to_c_map::const_iterator Sc_iterator;
typedef Sc_iterator Subconstraint_iterator;
Expand All @@ -186,7 +180,7 @@ class Polyline_constraint_hierarchy_2
public:
Polyline_constraint_hierarchy_2(const Compare& comp)
: comp(comp)
, sc_to_c_map(Pair_compare(comp))
, sc_to_c_map()
{ }
Polyline_constraint_hierarchy_2(const Polyline_constraint_hierarchy_2& ch);
Polyline_constraint_hierarchy_2(Polyline_constraint_hierarchy_2&&) = default;
Expand Down Expand Up @@ -290,7 +284,7 @@ template <class T, class Compare, class Point>
Polyline_constraint_hierarchy_2<T,Compare,Point>::
Polyline_constraint_hierarchy_2(const Polyline_constraint_hierarchy_2& ch)
: comp(ch.comp)
, sc_to_c_map(Pair_compare(comp))
, sc_to_c_map()
{
copy(ch);
}
Expand Down