Initial-ish code review.

[rburema]

Since the library didn't really exist yet before this point, we didn't really have to work with PR's. One downside is that you can't really review to work done in the same way though -- so have this 'issue' instead.

libUvula/src/unwrap.cpp

Line 403 in 4eb68c5

printf("\rError adding mesh\n");

• You really don't want print statements in a library! -- At least not ones that aren't opt-in (opt-out by default).
  I've noticed we're importing spdlog anyway, maybe should make use of that to make this into a debug thing.

libUvula/src/unwrap.cpp

Line 347 in 4eb68c5

for (size_t i = 0; i < vertices.size(); ++i)

• You can use ranges::enumerate here instead, I think.

libUvula/pyUvula/pyUvula.cpp

Line 20 in 4eb68c5

throw std::runtime_error("Vertices should be <float, float, float> and indices should be (grouped by face as) <int, int, int>.");

libUvula/pyUvula/pyUvula.cpp

Line 38 in 4eb68c5

throw std::runtime_error("Couldn't unwrap UV's!");

• These aren't caught in Uranium when called (see the review there...)

libUvula/src/unwrap.cpp

Line 98 in 4eb68c5

float best_outlier_angle = 1.0;

libUvula/src/unwrap.cpp

Line 103 in 4eb68c5

float face_best_angle = -1.0f;

• I know this'll be fine in practice, but I always get a little antsy if I see 'limit' values that could actually occur
  (like, it's technically possible to have a dot-product of 1.0 between two vectors).
  It's probably better to have something that's a little bigger than |1.0| exactly -- I might even have gone with (-)numeric_limits<float>::infinity().

libUvula/src/unwrap.cpp

Line 101 in 4eb68c5

for (auto iterator = unprocessed_faces.begin; iterator != unprocessed_faces.end; ++iterator)

• See https://deepwiki.com/ericniebler/range-v3/4.3-take-drop-and-slice-views#slice-views for an alternative.

libUvula/src/unwrap.cpp

Line 202 in 4eb68c5

for (auto iterator = projected_faces_groups.begin(); iterator != projected_faces_groups.end(); ++iterator)

• You can use structured binding to get (references to) both the key and value of an iterated map-item like so;
  for(auto [key, value] : projected_faces_groups) { ... }
  (Also; key and value will both be refs, no need to put auto& that'll just confuse the compiler.)

libUvula/src/unwrap.cpp

Line 255 in 4eb68c5

if (assigned1)

libUvula/src/unwrap.cpp

Line 286 in 4eb68c5

if (! assigned1)

• These are blocks with repeated code. It's not too bad, but consider something like;

   constexpr bool unassigned = false
   auto face_struct_list = {
     {face.i1, unassigned, new_indices_groups.find(face.i1) }
     {face.i2, unassigned, new_indices_groups.find(face.i2) }
     {face.i3, unassigned, new_indices_groups.find(face.i3) }
   }
  

  Then for either block you can use something like;

   ranges::for_each(face_struct_list,
   	[](auto& tuple)
   	{
   		auto [idx, assigned, it] = tuple;
   		// ... rest of loop-code
   	});
  

[rburema]

@wawanbreton

[wawanbreton]

Thanks, that makes a lot of sense ! At first I didn't have spdlog and ranges imported, in order to keep the lib as self-contained as possible, but finally I imported them and didn't use them everywhere possible. I will do it.

[rburema]

@wawanbreton No problem!

As for ranges, once we switch over to C++23 I think everything we use from ranges is also in the std (for C++20 I think there're too many missing elements that we end up using).

[wawanbreton]

As for ranges, once we switch over to C++23 I think everything we use from ranges is also in the std (for C++20 I think there're too many missing elements that we end up using).

It is the case indeed. At first I tried to use only the ranges features included in C++20, but I was missing especially enumerate. Now I think about it, it would have been interesting to try this isolated project in C++23 btw...

[wawanbreton]

Most of the changes are applied.
I did a modified version of your suggestion for the code duplication, with a more classic implementation (yes, I am still very restrictive about using auto)
For the ranges, I was very excited to use the views, but it turns out I can't really in this case, because I am iteratively reorganizing the vector with std::partition. I did replace my iterators structure by a ranges::subrange though, so it saves some lines and is more standard.

[rburema]

Most of the changes are applied. <...>

I think that's fine, almost all of the remarks where refactoring suggestions -- I'm just glad you gave it a try.