[Feature] Shape detection with region growing #3864
Conversation
|
@maxGimeno Can you help Dmitry on this PR? Travis fails, but I'm pretty sure it's something not code-related (but more of a config problem): this PR renames the package |
sgiraudot
changed the title
|
Yes. You need to go to the It will update the .travis.yml file, and then you can commit. |
|
I see. Thanks! |
Shape_detection/include/CGAL/Shape_detection/Efficient_RANSAC/Efficient_RANSAC.h
Outdated
Show resolved
Hide resolved
Shape_detection/include/CGAL/Shape_detection/Efficient_RANSAC/Efficient_RANSAC.h
Outdated
Show resolved
Hide resolved
Shape_detection/examples/Shape_detection/region_growing_on_point_set_2.cpp
Outdated
Show resolved
Hide resolved
Shape_detection/include/CGAL/Regularization/regularize_planes.h
Outdated
Show resolved
Hide resolved
…l_with_region_growing-danston
|
@maxGimeno Could you please have a look at this pull again? I generated travis as you said, but it anyways failed. I then also modified a few doc references of the packages which depend on my new package and they also started failing. What do I do wrong? Is there any tutorial on github how to do a pull request correctly? Thanks! |
|
Actually, reading the travis reports, i see that you include files that do not exist.
/home/travis/build/CGAL/cgal/Classification/examples/Classification/example_cluster_classification.cpp:16:10: fatal error:
And the CHECK test indicates that you failed to update travis.yml. |
|
When I try from your branch, I get this diff: |
|
@maxGimeno Aaah, ok, sorry I missed some logs, true. Thanks for pointing out. However, the PACKAGE check is still weird for me because I used the script generate_travis as you said and then committed it so I am still wondering why travis.yml is not updated. |
|
@danston I think it is updated, just the order of the packages is different for some reason. It should be alphabetical but I see it is not. Maybe it is platform related, and bash doesn't act the same way on macOS or something. Anyways, I pushed the fix . |
|
@maxGimeno Ok, thanks a lot. The order difference is strange. I will try to have a look at my system and see what could have gone wrong. |
…_growing-danston' of github.com:danston/cgal into Point_set_shape_detection_3-make_it_general_with_region_growing-danston
|
Travis reports: It means that when it compares the dependencies of the package in that branches to the ones reported in package)info, there are differences. |
|
BGL is a new dependency misisng in the file. Is it wanted ? |
|
Yes, I just did not have time to fix it yesterday. |
…e_it_general_with_region_growing-danston [Feature] Shape detection with region growing
lrineau
removed
the
rm only: ready for master
|
Ok, I will prepare such a news entry and let you know. |
|
The testsuite Ic-57 for VC++ fails. you have to replace @sloriot @sgiraudot : Shall one use |
|
|
|
I changed it in my fork, but the pull request is already closed. How should I do it now? Thanks. |
|
@maxGimeno commented on Jun 3, 2019, 10:15 AM GMT+2:
@maxGimeno Please be careful when you mark pull-requests as tested. The results for CGAL-5.0-Ic-49 were incomplete: with any tests on Windows. @danston commented on Jun 12, 2019, 11:00 AM GMT+2:
Dmitry, you will have to open a new pull-request. It can use the same branch you already have. |
|
@lrineau Ok, clear. I will open a new one. But how come the test suite is incomplete? I checked it and everything was green. Do I miss smth? |
|
Can Maxime not do that directly in master? |
|
No. I can't push in master. |
|
Here is the new pull request: #3994 |
|
@danston commented on Jun 12, 2019, 11:47 AM GMT+2:
It was incomplete because that day some of the test platforms had network or power issues, and had not sent their results. |
|
@lrineau Ok, I see. Next time, I will wait longer before reporting my results. Thanks for clarifying. |
|
You don't have to report your results, I check the testsuite every morning and report the points of interest. We were just unlucky to not notice the missing platforms this time. |
Bump! :) |
|
I remember about the news entry. I still need to finish a few other things and then I will immediately prepare a picture and a short description, just the picture I want to do requires a bit of coding :) Thanks for the patience. |
|
Online there: https://www.cgal.org/2019/07/30/Shape_detection/ |
| - `RegionType::update()` - updates the internal flag from the default `false` to `true`. | ||
|
|
||
| We also define a `SeedMap`, such that the second object is handled first, while the first object follows. | ||
| Moreover, the last object is always skipped. Notice that in this example, the container with objects is `std::list`, |
There was a problem hiding this comment.
Notice that in this example, the container with objects is
std::list
That's not the case region_growing_with_custom_classes.cpp uses a vector: std::vector<std::size_t> neighbors;
Yet the code traverses the vector as if it was a linked list:
void operator()(
const std::size_t query_index,
std::vector<std::size_t>& neighbors) const {
std::size_t i = 0;
for (const auto& object : m_objects) {
if (i == query_index) {
neighbors = object.neighbors;
return;
} ++i;
}
}That is rather confusing.
Perhaps it once was a list but then the type was changed to a vector before the PR was submitted/merged, without updating the algorithm and docs?
There was a problem hiding this comment.
@nh2 Hi. You are completely right! Thank you for this remark! It is my fault. Indeed, I first used std::list and then updated to std::vector, because some reviewers did not like using the list, but I forgot to update the docs for some reason. I will fix it. However, note that RG can still work with std::list. In general, std::vector is not required, it is used only because it is more efficient.
There was a problem hiding this comment.
@danston Makes sense!
Another quick question:
From the added docs, on region growing, it doesn't seem to become quite clear how max_accepted_angle works. The docs say:
angle_threshold- the maximum accepted angle between the normal associated with a point and the normal of a line/plane;
This suggests that the angle is indeed between each individual face and the fitted plane. However, I have observed the algorithm to produce two things that seem impossible under that interpretation, using face-based plane fitting:
- With an
angle_thresholdof 90: When I project associated faces onto the fitted plane, some project normals have opposite signum to the plane's normal - With an
angle_thresholdof 70: Two noisy parallel planes facing each other being detected as part of the same region (shouldn't that be impossible, given that their face normals pretty much look at each other?)
I haven't fully read all implementation yet; maybe you can give me a quick hint at whether that's intended, and how we could make the documentation best reflect what's happening?
There was a problem hiding this comment.
I think I found the reason for the above:
In
we have abs() which throws away the sign of the normal, thus also permitting faces that are within 180 <= here < 180-angle_threshold degrees!
E.g. assume your fit plane's normal points exactly upwards. If you set the angle_threshold to, say, 5 degrees, this will not permit angles that are "pretty much exactly upwards" but also "pretty much exactly downwards".
Is that intended?
There was a problem hiding this comment.
I noticed another problem: The plane's normal is not estimated correctly (it is arbitrary, likely depending on the position of the plane relative to the origin).
I've put a fix for the above 2 issues into PR #4563.
In CGAL#3864 region growing was introduced, but the `linear_least_squares_fitting_3()` used in `update()` cannot estimate a correct face normal direction from just unoriented vertex points (the plane normal's orientation signum may be flipped). This commit flips the estimated plane normal to face into the same direction as the majority of the faces it is estimated from. With the estimated planes now being "the right way around", this commit subsequently also fixes the incorrect use of `abs()` in `is_part_of_region()` when comparing a face's normal to the estimated plane normal (direction matters here). This fixes parallelly-opposing planar regions (means: with opposite normals), connected with some triangles in between, like this: | -> <- | | -> <- | Legend: | -> <- | face: | | -> <- | normal: -> | -> <- | \__________/ being detected as 1 region. This should have been impossible according to the docs ("... and if the angle between its normal and the plane's normal is within the `angle_threshold`"), given the documented requirement ("\pre `angle_threshold >= 0 && angle_threshold <= 90`"). Now it is impossible.
In CGAL#3864 region growing was introduced, but the `linear_least_squares_fitting_3()` used in `update()` cannot estimate a correct face normal direction from just unoriented vertex points (the plane normal's orientation signum may be flipped). This commit flips the estimated plane normal to face into the same direction as the majority of the faces it is estimated from. With the estimated planes now being "the right way around", this commit subsequently also fixes the incorrect use of `abs()` in `is_part_of_region()` when comparing a face's normal to the estimated plane normal (direction matters here). This fixes parallelly-opposing planar regions (means: with opposite normals), connected with some triangles in between, like this: | -> <- | | -> <- | Legend: | -> <- | face: | | -> <- | normal: -> | -> <- | \__________/ being detected as 1 region. This should have been impossible according to the docs ("... and if the angle between its normal and the plane's normal is within the `angle_threshold`"), given the documented requirement ("\pre `angle_threshold >= 0 && angle_threshold <= 90`"). Now it is impossible.
In CGAL#3864 region growing was introduced, but the `linear_least_squares_fitting_3()` used in `update()` cannot estimate a correct face normal direction from just unoriented vertex points (the plane normal's orientation signum may be flipped). This commit flips the estimated plane normal to face into the same direction as the majority of the faces it is estimated from. With the estimated planes now being "the right way around", this commit subsequently also fixes the incorrect use of `abs()` in `is_part_of_region()` when comparing a face's normal to the estimated plane normal (direction matters here). This fixes parallelly-opposing planar regions (means: with opposite normals), connected with some triangles in between, like this: | -> <- | | -> <- | Legend: | -> <- | face: | | -> <- | normal: -> | -> <- | \__________/ being detected as 1 region. This should have been impossible according to the docs ("... and if the angle between its normal and the plane's normal is within the `angle_threshold`"), given the documented requirement ("\pre `angle_threshold >= 0 && angle_threshold <= 90`"). Now it is impossible.
…erated mesh; fixed docs; cleanup
…erated mesh; fixed docs; cleanup
Shape detection with generic region growing - version 1.0
This submission proposes a major update to the
Point_set_shape_detection_3package by extending it with the generic region growing algorithm that works on a set of arbitrary items. We contribute the generic version of the region growing algorithm and its three particular instances: region growing on points in 2D and 3D, and region growing on a polygon mesh.Summary of Changes
Status
Release Management
Point_set_shape_detection_3TODO: