examples: Shape_detection: Loading the points the Python way

[thopiekar]

Personally, I find it very confusing that it is allowed to load points by passing a path to Point_set_3().
Logically (from what I know from Python) I would expect that open("myfile"), which is a data stream, can be passed instead.

Anyway, this rewritten code should work alternatively as far as I understand the documentation of CGAL.
However, I get an segmentation fault when running the example like this.

This commit is therefore a more pythonic example and a bug report at the same time.

Signed-off-by: Thomas Karl Pietrowski [email protected] (github: thopiekar)

[sgiraudot]

I'm opposed to this PR.

I agree with the analysis: the fact that the reading function takes a file name instead of a data stream was done because it was simpler for us to handle the file IO from the C++ side. But indeed, it would be much better to use a stream (similarly to what is done in C++). I don't know how hard it is to bind a Python stream to a C++ STD stream, but I think it's a good idea to look into it, so again, I agree with the analysis, it'd be better to be able to do:

point_set = Point_set_3()
with open(“filename”) as file:
    point_set.read(file)

Nevertheless, focusing now on the content of this PR: I really don't see how it would be an upgrade to replace a single line that does the work of reading the file by 25 lines that basically encourage users to parse the files by themselves.

[thopiekar]

Well, it reproduces crashes I get when getting the values from somewhere else. I'm trying to use your bindings within a software, which already read all mesh data. When I feed your Point_set_3() with data as I do here, I get segmentation faults, too.
So a workaround might be to create a temporary pwn file as you have in the examples and then let your bindings read it. However, going this indirect way isn't something I'd like to do.

So back to the PR here: Yes, parsing and analysing the file like this is not the most efficent way, but doing the steps as done in line 24, 25 and 26 is probably how many other people would integrate your bindings in other software and it causes a segmentation fault as you can see in the CI, too.

[sgiraudot]

Well, it reproduces crashes I get when getting the values from somewhere else. I'm trying to use your bindings within a software, which already read all mesh data. When I feed your Point_set_3() with data as I do here, I get segmentation faults, too.

First: sorry if this wasn't clear from my comment in #158, but there is no bug here, the crash you get is from a misuse of the package (as I explained, you need to add the normal map before inserting normals).

So back to the PR here: Yes, parsing and analysing the file like this is not the most efficent way, but doing the steps as done in line 24, 25 and 26 is probably how many other people would integrate your bindings in other software

Which is exactly what is covered in the Point_set_3 example (which also covers the normal map addition by the way).

The example here is not about feeding existing data to a Point_set_3, it's about shape detection and it uses a file, for which using the file reading function is the way to go.

[thopiekar]

As mentioned in the previous issue ticket (which has been renamed now), I added the missing normal map. The example works as intended now.

It is up to you whether to merge it or not. 😄

[lrineau]

I'm opposed to this PR.

I agree with the analysis: the fact that the reading function takes a file name instead of a data stream was done because it was simpler for us to handle the file IO from the C++ side. But indeed, it would be much better to use a stream (similarly to what is done in C++). I don't know how hard it is to bind a Python stream to a C++ STD stream, but I think it's a good idea to look into it, so again, I agree with the analysis, it'd be better to be able to do:

point_set = Point_set_3()
with open(“filename”) as file:
    point_set.read(file)

@sloriot Have you already wrapped any C++ I/O functions, in Python bindings?

[sloriot]

I always used a filename because it is working in all target language, I never looked for an alternative.