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.

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

[Apps] Remove pcl_openni_face_detector #5513

Conversation

SunBlack
Copy link
Contributor

Remove app pcl_openni_face_detector, as the example files are missing since years.

The file forest.txt seems to be required, but I cannot find file file anywhere. This was already a topic some years ago: #2169

I tried to find the file via git log --all --full-history -- "*forest.txt", but it seems this file never existed in this repo. Even in the data repo I could not find these files.

As the app is crashing without the file, removed the whole app. As it is a executable an not library, we should not need an deprecation cycle here.

@mvieth
Copy link
Member

mvieth commented Nov 15, 2022

In the data repo you linked, there are several files with the word "forest" in the name (compressed with "bz2" filename extension). Couldn't they work as input files?

@SunBlack
Copy link
Contributor Author

@mvieth Yeah I saw them. Didn't tried them until now, as there is also a face.pcd, which seems to be optional, but is also missing. The forest bz2 files seems to be from gpu/people module and not from the apps (see f07c4e3), but I can try it, if these files are readable by the app.

@SunBlack
Copy link
Contributor Author

Ok tried it now with forest1/tree_20.txt, but stopped loading at 50GB memory usage 😆 I don't understand how this code should have ever worked (even I take a revision from 2012). The default suggest, that it is a text file, not a binary file. But the method DecisionForest::deserialize seems to expect a binary file (see the call to std::istream::deserialize)

@mvieth
Copy link
Member

mvieth commented Dec 27, 2022

@SunBlack I tested the biwi_face_database in the data repo (forest_example.txt) and it works fine (except that I don't have a device connected, but the loading of the forest works). There is even a model.pcd (for model_path_). So there is no reason to remove this code. Instead I would suggest to

  • add a hint/link to the biwi_face_database in the code or similar
  • remove fdrf.setForestFilename(forest_fn); since that would only be relevant if fdrf.trainWithDataProvider() was called
  • check the return value of fb.open(forest_fn.c_str(), std::ios::in) and exit on failure (e.g. if the given file does not exist)

@mvieth mvieth added the needs: author reply Specify why not closed/merged yet label Jan 4, 2023
@mvieth
Copy link
Member

mvieth commented Jan 26, 2023

@SunBlack What do you think about my earlier suggestions?

@SunBlack
Copy link
Contributor Author

@SunBlack What do you think about my earlier suggestions?

So far I haven't had time to deal with the PCL again (usually I have time for it when most of the others are on vacation).

Provided the app still works, we might indeed keep it. In principle, however, it would be good to create an overview somewhere, which demo should show what exactly, because I partly have the feeling that large parts of the apps are just copy & paste and only an algorithm was exchanged. The question is whether the demos should show the code or the results - in the latter case a larger combined app would probably make more sense to avoid code duplications. Since no one knew how the app still worked, the question would be which category it belongs to in the first place.

@mvieth
Copy link
Member

mvieth commented Feb 3, 2023

@SunBlack That is a good point. We have programs in the apps/ folder, in the tools/ folder, in the examples/ folder, in the doc/tutorials/ folder, and probably in even more places. Some of the programs are supposed to show how to use PCL classes in code (these are mostly the tutorials), some are meant as every-day-tools (e.g. converting between different file formats or downsampling a pcd file), and some are perhaps meant to show what functionality PCL has (but not specifically how to use it in code).

... or the results - in the latter case a larger combined app would probably make more sense to avoid code duplications

This may make sense for some apps, but just throwing everything in one big cpp file can't be the solution either. Maybe put common functionality in a header file that all apps can include?

Since no one knew how the app still worked, the question would be which category it belongs to in the first place.

I would say it is mostly meant to show the results/show what functionality PCL has.

Let's continue discussion about which app/tool/program should serve what purpose, where they should be found, and how to reduce code duplication, in a separate issue. And when you have time, please apply my earlier suggestions (if you agree with them, of course), so we can merge this 😄

@mvieth
Copy link
Member

mvieth commented Apr 17, 2023

Superseded by #5669

@mvieth mvieth closed this Apr 17, 2023
@SunBlack SunBlack deleted the remove_pcl_openni_face_detector branch June 7, 2023 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module: apps needs: author reply Specify why not closed/merged yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants