[REVIEW]: A Structured-Light Scanning Software for Rapid Geometry Acquisition about joss-reviews HOT 114 CLOSED

✋ I will review this weekend.

from joss-reviews.

If example data is provided and the software is a tool for post-processing this data (i.e. the software and its claims do not rely on or control the hardware) then we can still review this submission. @charalambos for those interested in testing new objects, can you provide a link to/description of the required hardware?

from joss-reviews.

Great. Once you fixed those remaining issues let me know and I'll inform the reviewer to pick up where we left off. Thanks.

from joss-reviews.

/ cc @openjournals/joss-reviewers - would anyone be willing to review this submission?

If you would like to review this submission then please comment on this thread so that others know you're doing a review (so as not to duplicate effort). Something as simple as :hand: I am reviewing this will suffice.

Reviewer instructions

• Please work through the checklist at the start of this issue.
• If you need any further guidance/clarification take a look at the reviewer guidelines here http://joss.theoj.org/about#reviewer_guidelines
• Please make a publication recommendation at the end of your review

Any questions, please ask for help by commenting on this issue! 🚀

from joss-reviews.

Does the reviewer need to be a scientist at a non-profit institution? I have a colleague trained as an anthropologist who now runs a 4D scanning company who might be able to review.

from joss-reviews.

Does the reviewer need to be a scientist at a non-profit institution? I have a colleague trained as an anthropologist who now runs a 4D scanning company who might be able to review.

@davclark no, I think your colleague sounds good.

from joss-reviews.

Thanks for jumping in for JOSS, @davclark !!

from joss-reviews.

And thanks for the invite :)

from joss-reviews.

My colleague may or may not have time for this, but he did point out that this paper also has the issue of difficulty of full verification without necessary hardware. So it seems openjournals/joss#156 is relevant here also.

from joss-reviews.

That is correct in the case where you would like to scan a new object. For testing however, we provide a complete dataset (Alexander folder) which you can use.

from joss-reviews.

To process the example data. The only hardware required is a nVidia graphics card supports CUDA. We used Geforce GTX770 in our development.

from joss-reviews.

@Kevin-Mattheus-Moerman We haven't completed the detailed users' manual for this version yet, but the procedure for setting up and scanning described in the previous version (http://www.3dunderworld.org/wp-content/uploads/2014/03/3DUNDERWORLD-SLSv3.0.pdf) is the same. Please note that v4 has no dependency on CANON's EOS API

from joss-reviews.

How large it the git repo? I interrupted at 100Mb. Usually it is a good idea to have the data separate from the sources.

from joss-reviews.

@pjotrp The repository is 299.04MB. I have no problem to clone the repository. But you are right that it is better to have the data separated.

from joss-reviews.

OK, put a warning on the size in the README, that should suffice for now.

from joss-reviews.

@whedon commands

from joss-reviews.

Here are some things you can ask me to do:

# List all of Whedon's capabilities
@whedon commands

# Assign a GitHub user as the reviewer of this submission
@whedon assign @username as reviewer

# List the GitHub usernames of the JOSS editors
@whedon list editors

# List of JOSS reviewers together with programming language preferences and domain expertise
@whedon list reviewers

# Change editorial assignment
@whedon assign @username as editor

# Set the software archive DOI at the top of the issue e.g.
@whedon set 10.0000/zenodo.00000 as archive

# Open the review issue
@whedon start review

🚧 Important 🚧

This is all quite new. Please make sure you check the top of the issue after running a @whedon command (you might also need to refresh the page to see the issue update).

from joss-reviews.

@whedon list editors

from joss-reviews.

Current JOSS editors:

@acabunoc
@arfon
@cMadan
@danielskatz
@jakevdp
@karthik
@katyhuff
@kyleniemeyer
@labarba
@mgymrek
@pjotrp
@tracykteal

from joss-reviews.

@whedon assign @Kevin-Mattheus-Moerman as editor

from joss-reviews.

OK, the editor is @Kevin-Mattheus-Moerman

from joss-reviews.

Dear authors @charalambos. Apologies for the delay with this submission. I'm in the process of finding reviewers for your submission. If you are able to suggest reviewers please do so as well. Thanks.

from joss-reviews.

@Kevin-Mattheus-Moerman Thank you for letting us know.

from joss-reviews.

@simonfuhrmann @kangxue @mikhail-matrosov @daeyun @lasvegasrc @daviddoria @krm15 @kysucix is this your cup of tea? Are you willing to sign up as a JOSS reviewer and review this submission?

Link to the submission in JOSS:
#47

The project GitHub repository:
https://github.com/theICTlab/3DUNDERWORLD-SLS-GPU_CPU

The paper:
https://github.com/openjournals/joss-reviews/files/402839/10.21105.joss.00047.pdf

The submission's project website:
http://www.3dunderworld.org/

Reviewer instructions:
http://joss.theoj.org/about#reviewer_guidelines

from joss-reviews.

@charalambos Some editorial comments on your paper.
Could all authors share the same affiliation index? (currently affiliations are copied 3 times in paper).
In your paper you mention:

We have performed extensive testing with a wide range of models and the results are documented in our report.

Could you expand on this please. This would be very helpful for the current reviewers but also future readers. Where are the models? What sort of models are these? What is the "report", do you mean the documentation? Could you even say something like "The README will guide the user to a suite of test models which allow the user to replicate typical results" (adjust as you see fit)

from joss-reviews.

@daviddoria Thanks! Welcome on board Sir!

from joss-reviews.

@whedon assign @daviddoria as reviewer

from joss-reviews.

I'm sorry @Kevin-Mattheus-Moerman, I'm afraid I can't do that. That's something only JOSS editors are allowed to do.

from joss-reviews.

@arfon Can you check what is wrong here?

from joss-reviews.

@rougier do you know an additional reviewer at INRIA perhaps?

from joss-reviews.

@whedon list editors

from joss-reviews.

Current JOSS editors:

@acabunoc
@arfon
@biorelated
@cMadan
@danielskatz
@jakevdp
@karthik
@katyhuff
@Kevin-Mattheus-Moerman
@kyleniemeyer
@labarba
@mgymrek
@pjotrp
@tracykteal

from joss-reviews.

@Kevin-Mattheus-Moerman you should be good to go now.

from joss-reviews.

@whedon assign @daviddoria as reviewer

from joss-reviews.

@whedon assign @daviddoria as reviewer

from joss-reviews.

@whedon assign @daviddoria as reviewer

from joss-reviews.

OK, the reviewer is @daviddoria

from joss-reviews.

All,

Please excuse the "newbie" questions as I'm new to this process :).

I started checking boxes at the top of this issue for things that I verified. Presumably for things that I do NOT check there should be somewhere to explain why I did not check them?

I am having trouble building the project. I have listed the first two issues below - is this where I should discuss these things? Can/should I contact the author directly to get help like this and then ensure the documentation is updated?

1. This error:
   /home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/src/lib/GrayCode/GrayCode.hpp:2:30: fatal error: opencv2/opencv.hpp: No such file or directory
   #include <opencv2/opencv.hpp>

seems resolvable by adding a cmake line:
INCLUDE_DIRECTORIES(${OpenCV_INCLUDE_DIRS})

2. I then get this error:

In file included from /home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/./src/lib/core/Camera.h:24:0,
from /home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/./src/lib/core/fileReader.h:19,
from /home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/./src/lib/calibration/Calibrator.hpp:2,
from /home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/src/lib/calibration/Calibrator.cpp:1:
/home/doria/source/3DUNDERWORLD-SLS-GPU_CPU/./src/lib/core/Dynamic_Bitset.h:29:23: fatal error: glm/glm.hpp: No such file or directory
#include <glm/glm.hpp>

I tried apt-get install glm-dev but that does not seem to be a package (Ubuntu 14.04).

Thanks,

David

from joss-reviews.

@Kevin-Mattheus-Moerman FYI the link you posted in your post "is this your cup of tea? Are you willing to sign up as a JOSS reviewer and review this submission?" links to https://github.com/openjournals/joss-reviews/issues/joss.theoj.org which seems to be broken.

from joss-reviews.

@charalambos The link to the paper https://github.com/openjournals/joss-reviews/files/402839/10.21105.joss.00047.pdf is an extremely short document with no images, etc. However I see in your /doc folder that there are a bunch of image files. Is there a more "real" document that I'm missing?

from joss-reviews.

@daviddoria GLM can be installed with sudo apt install libglm-dev on Ubuntu.

from joss-reviews.

This does not build against OpenCV3, which is fine, but it just needs to indicate the versions of the dependencies somewhere.

from joss-reviews.

I saw this in Line 14 of Calibrator.hpp:

static std::condition_variable cv;

That seems really fragile given that there is the 'cv' namespace for OpenCV.

from joss-reviews.

It seems as though an "in source" build is expected (which is almost never recommended). I say that because:

1. The readme says "Binaries are compiled and located in the bin folder." which presumably is implying [sourceTree]/bin.

2. When trying to run the executable (I cloned in ~/source/underworld and built in ~/build/underworld), I get:

doria@doria-home:~/build/underworld$ ./bin/SLS
No image read from ../../data/alexander/rightCam/dataset1/ ... No image read from ../../data/alexander/leftCam/dataset1/ ... Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Segmentation fault (core dumped)

which seems to be referencing files relative to the build tree (which is expected to be the source tree). This is bad - these paths should be specifiable on the command line.

from joss-reviews.

@daviddoria thanks again for reviewing this submission. To answer your questions.

I started checking boxes at the top of this issue for things that I verified. Presumably for things that I do NOT check there should be somewhere to explain why I did not check them?
I am having trouble building the project. I have listed the first two issues below - is this where I should discuss these things? Can/should I contact the author directly to get help like this and then ensure the documentation is updated?

Yes, please post any comments and issues you want to communicate to the authors here. This way the entire review process is open and traceable.

from joss-reviews.

@daviddoria in relation to

@charalambos The link to the paper https://github.com/openjournals/joss-reviews/files/402839/10.21105.joss.00047.pdf is an extremely short document with no images, etc. However I see in your /doc folder that there are a bunch of image files. Is there a more "real" document that I'm missing?

Papers in JOSS are deliberately short. See some of our motivation behind it here:
http://www.arfon.org/announcing-the-journal-of-open-source-software

Here are are paper content requirements: http://joss.theoj.org/about where it says

JOSS papers should contain the following:

• A list of the authors of the software
• Author affiliations
• A short summary describing the high-level functionality of the software
• A list of key references including a link to the software archive

and see some examples of accepted papers here:
http://joss.theoj.org/papers/popular

So the "real" document you refer to (you probably mean a classic full length paper) is not required if the software documentation is complete enough. So if you find the descriptions are lacking feel free to also demand expanded documentation.

from joss-reviews.

@daviddoria authors are allowed to include figures in the paper however so of course you can also recommend that if you feel it is needed.

from joss-reviews.

@daviddoria There is also a "traditional" paper that goes along with the code and described the technique at arxiv: http://arxiv.org/abs/1406.6595

There is also a user's manual for setting up and using the software. This was written for a previous version but the setup is identical: http://www.3dunderworld.org/wp-content/uploads/2014/03/3DUNDERWORLD-SLSv3.0.pdf

from joss-reviews.

@charalambos
Thanks for the link to the traditional paper. The setup instructions still do not seem to list anything about prerequisites (which version of cmake/opencv/etc is required, which packages likely need to be installed on common platforms (the GLM issue I had), etc.

Did you see my comment about the assumption about an in-source build? I was going to say that is blocking me from continuing, but suppose I could do an in-source build to continue the review for now... haha

@Kevin-Mattheus-Moerman - is it appropriate to put a link to the arXiv paper in the JOSS paper? It's really hard to start a review without knowing what the code is even supposed to be implementing.

from joss-reviews.

@daviddoria
Yes, I saw your comment on the code. Sorry I didn't get chance to touch the code last weekend, I'll fix it later tonight.

from joss-reviews.

@charalambos @daviddoria the paper may cite other and previous works. However the software submission for JOSS should stand-alone i.e. we cannot simply refer to another paper and old documentation from a previous version.

I would prefer if the authors did the following.

1. Expand the paper to give a more complete description of the software functionality. Consider including an overview figure to graphically show the software capabilities. The paper can remain short but should give readers and reviewers enough information to understand the capabilities of the software. As long as the JOSS paper provides a sufficient "stand-alone" description, feel free to also refer to previous work/papers.
2. Please fix the documentation based on the reviewers comments. Also please update the documentation for the version submitted here. If the documentation for the previous version is indeed very similar it should be little work to update.

from joss-reviews.

@charalambos @v3c70r any progress on addressing the comments by myself and the reviewer?

@daviddoria thanks again for reviewing this submission. Could you update me on whether you are currently continuing actively reviewing content or if you are waiting for action by the authors? Thanks.

from joss-reviews.

@daviddoria @Kevin-Mattheus-Moerman Yes.

1. Now data and configuration files are passed as parameters. The default of the parameters can be used when the bin is compiled in [sourceTree]/bin.
2. The readme documents has been updated to contain more information, including the purpose of the tool, an instructions of the sample code and an example of using the library.

from joss-reviews.

@Kevin-Mattheus-Moerman I was waiting for author action. It sounds like the things I was waiting for are now available, so I'll take another look this weekend.

from joss-reviews.

@daviddoria great. Thanks for the update.

from joss-reviews.

@v3c70r Thanks, it seems to build now, and the summary in the readme is much much better. I tried to produce the help message, but I get:

doria@doria-home:~/build/underworld$ ./bin/SLS -h
terminate called after throwing an instance of 'std::regex_error'
what(): regex_error
Aborted (core dumped)

from joss-reviews.

@v3c70r Actually, I'm getting that no matter what I try:

doria@doria-home:~/build/underworld$ ./bin/SLS
terminate called after throwing an instance of 'std::regex_error'
what(): regex_error
Aborted (core dumped)

doria@doria-home:~/build/underworld$ ./bin/SLS -l ~/source/3DUNDERWORLD-SLS-GPU_CPU/data/alexander/leftCam/dataset1/ -L ~/source/3DUNDERWORLD-SLS-GPU_CPU/data/alexander/leftCam/calib/output/calib.xml -r ~/source/3DUNDERWORLD-SLS-GPU_CPU/data/alexander/rightCam/dataset1/ -R ~/source/3DUNDERWORLD-SLS-GPU_CPU/data/alexander/rightCam/calib/output/calib.xml -o test.ply
terminate called after throwing an instance of 'std::regex_error'
what(): regex_error
Aborted (core dumped)

Oddly, I put a breakpoint on the very first line in main() of App.cpp and even before it gets there I get a SIGABRT in /usr/include/c++/4.8/bits/regex_compiler.h (line 974: __throw_regex_error(regex_constants::error_brack);)

Note that this is gcc 4.8 (stock from Ubuntu 14.04):

doria@doria-home:/build/underworld$ g++ --version
g++ (Ubuntu 4.8.4-2ubuntu114.04.3) 4.8.4

from joss-reviews.

@daviddoria Hmm, it seems no problem on my machine. What compiler are you using?

from joss-reviews.

@v3c70r I commented everything in main() and the error was still there. I commented the headers one at a time and as soon as I stop including cppopts.hpp the error goes away. Might I recommend using boost::program_options instead of 1400 lines of custom parsing? (see my compiler and system in my previous comment)

from joss-reviews.

@daviddoria Right, I'm removing that header and use cmdline.h instead.

from joss-reviews.

@daviddoria Updated, the command line parser should work now.

from joss-reviews.

@v3c70r That worked. Next comment - it finally built and started to run, but I got:

Starting /home/doria/build/underworld/bin/SLS...
OpenCV Error: Unspecified error (The function is not implemented. Rebuild the library with Windows, GTK+ 2.x or Carbon support. If you are on Ubuntu or Debian, install libgtk2.0-dev and pkg-config, then re-run cmake or configure script) in cvWaitKey, file /home/doria/source/opencv-2.4.13/modules/highgui/src/window.cpp, line 567
terminate called after throwing an instance of 'cv::Exception'
what(): /home/doria/source/opencv-2.4.13/modules/highgui/src/window.cpp:567: error: (-2) The function is not implemented. Rebuild the library with Windows, GTK+ 2.x or Carbon support. If you are on Ubuntu or Debian, install libgtk2.0-dev and pkg-config, then re-run cmake or configure script in function cvWaitKey

The program has unexpectedly finished.

Luckily, the error told me exactly how to fix it. However, this should be caught at configure time, not run time (surely there's a CMake query to ask OpenCV if it has whatever GTK stuff you require).

It did seem to run to completion now, so I can continue the review!

from joss-reviews.

@v3c70r I tried to view the output .ply file in Paraview (5.1) and got:

ERROR: In /home/kitware/dashboards/buildbot/paraview-debian6dash-linux-shared-release_opengl2_qt4_superbuild/source-paraview/VTK/IO/PLY/vtkPLYReader.cxx, line 138
vtkPLYReader (0x31a7780): Cannot read geometry

It did open OK in Meshlab 1.3.2 - but I really doubt it's a bug in VTK's PLY reader, so it might be worth looking into if something is non-standard in your file (and Meshlab is "accidentally" reading it correctly?)

from joss-reviews.

@daviddoria Thank you for your input. The ply file works fine with Blender.
, I will investigate the mesh exporter and do some more tests.

from joss-reviews.

@v3c70r Please find some comments below. Recall this is my first review, so if these types of comments are not what we're looking for, then someone please say so :)

• There are several key functions (like Calibrate(...) in Calibrator.hpp that do not have descriptions (the class itself should also have a description).
• All of the App*.cpp files need descriptions (and probably better names - perhaps using action verbs? e.g. App_Calib.cpp -> CalibrateImages.cpp, App.cpp -> ComputeReconstruction.cpp).
• The classes in the core/ folder are not well commented. E.g. (but this really applies to all of them) Camera.h and fileReader.h have tons of functions with no description. They are definitely not "obvious" either - I am left wondering why the Camera class has a mask, what a white/black threshold is, what kind of distortion 'undistort()' is correcting, what kind of configFile it can read, etc. I also am wondering why the fileReader class has getRay accessors, what kind of files it reads, why it seems to duplicate the Camera classes "white threshold" accessor, etc.
• (minor) - there doesn't seem to be a convention for file naming. There is log.hpp (all lower case), Camera.h (first character upper case), Dynamic_Bitset.h (underscore between words), fileReader.h (no underscore between words), etc.
• From looking at Reconstructor.h, I don't understand the API. What does it mean to add a camera to this object? It's not clear that pointCloud_ (I think) is an output object. I don't know what a Projector is. what is idx_? Why does reconstruct() not return the point cloud? etc.
• The GrayCode class has zero comments. Why is it in its own folder and not just in core/?
• (minor) a few files (Ray.h, Projector.h) do not show up in my QtCreator project. Simply listing them in core/CMakeLists.txt like this will fix that:
  add_library(core ${SOURCES} Ray.h Projector.h)
• CRITICAL: there do not seem to be any unit tests at all. This is very very bad. Since you include the nice Alexander data set, at the very least you could execute the reconstruction and compare the points that are produced to a "known-good" baseline reconstruction.

My overall feeling is that this code would be very very hard for someone to extend. I would like the core reconstruction algorithm to be heavily documented (if ReconstructorCPU::reconstruct() is even the right place to be looking, there are nearly zero comments).

from joss-reviews.

Review on commit d9786db.

Greetings, everyone!

I tried to build the code, here's what i faced:

1. cmake didn't check if libglm-dev was installed.
2. make command failed probably because of wrong GCC options (btw, i have Ubuntu 15.10):

make
[  6%] Building NVCC (Device) object src/lib/ReconstructorCUDA/CMakeFiles/sls_gpu.dir/sls_gpu_generated_ReconstructorCUDA.cu.o
/usr/lib/gcc/x86_64-linux-gnu/4.9/include/stddef.h(432): error: identifier "nullptr" is undefined
/usr/lib/gcc/x86_64-linux-gnu/4.9/include/stddef.h(432): error: expected a ";"
...<another hundred lines of errors>...

I also tried GCC 5.2.1 and 4.7.4 with the same result. Suggest that cmake should at least check compiler's version. A much better solution would be to fix the code so it can be build with GCC 5.

Also it would be great if it was possible to build the code without CUDA (even if it was detected in the system).

from joss-reviews.

I've tried to compile the code, couldn't yet do it. Getting the same error as @mikhail-matrosov. I'll try later on another PC without CUDA, meanwhile here are my thoughts on Reviewer questions:

Conflict of interest

YES As the reviewer I confirm that there are no conflicts of interest for me to review this work (such as being a major contributor to the software).

General checks

YES Repository: Is the source code for this software available at the repository url?
YES License: Does the repository contain a plain-text LICENSE file with the contents of an OSI approved software license?
YES Version: Does the release version given match the GitHub release (v4.0.0)?

Functionality

NO Installation: Does installation proceed as outlined in the documentation? - getting errors, couldn't fix them so far. Also, as mentioned above, cmake doesn't check if glm is installed.
YES Functionality: Have the functional claims of the software been confirmed? - the software works fine with the test dataset. GrayCode projection also works fine.
YES Performance: Have any performance claims of the software been confirmed?

Documentation
I've written this based on this: http://www.3dunderworld.org/wp-content/uploads/2014/03/3DUNDERWORLD-SLSv3.0.pdf and this: https://arxiv.org/pdf/1406.6595v2.pdf. Although, it is hard to call the paper "documentation", would be nice to have a separate documentation file. Also, the README.md file has unique information compared to previous two files. Again, it would be better to have a single full documentation file with detailed description on how to install, run and extend the software with some examples, and README.md should be just a short version of that doc.
YES A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
NO Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution. - couldn't find them in any of these two files.
PARTLY Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems). - There is an explanation on how to use in README.md file and different explanation in documentation file. No single place with detailed description.
PARTLY Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)? - One can find most information about functionality after reading the paper, docs and README.md, but it will be much better to present everything in a more structured way in a single doc.
NO Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified? - I don't really think this is important, especially since the test dataset is available and it works well.
NO/YES/YES Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support - it is hard to contribute without structured description of all the parts of the software and without extensively commented code. For 2) and 3) I guess Github's "Issues" section is good enough.

Software paper

Paper PDF: 10.21105.joss.00047.pdf

YES Authors: Does the paper.md file include a list of authors with their affiliations?
YES A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
NO References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)? - couldn't find the link to original paper.

Overall, I think that making the software more robust to different configurations and better structured documentation are main things to do for now. Developing good Structured Light software is a good goal, and, when it will be ready and well documented, it will be quiet useful for the community.
Also, it may be a good idea to add photo capture process through something like gphoto2 for arbitrary camera.

UPD. I was able to run the software on another PC without CUDA. Updated the review accordingly.

from joss-reviews.

@vaheta @davclark Thank you so much for your review, they are very helpful. I'll update the code accordingly this weekend.
@vaheta @mikhail-matrosov It looks like the nvcc c++11 is not explicitly enabled in cmake script. I will fix it.

Also it would be great if it was possible to build the code without CUDA (even if it was detected in the system).

It's a good idea, I'll add the option.

from joss-reviews.

@v3c70r can you give us an update on progress? Are you able to address the issues raised above?

from joss-reviews.

Yes, I'm currently working on the issues and have addressed some of them. But I need more time to finish all of them since I'm currently have a full-time job and work on the project in free time.

Here is the update:

• NVCC flags have been defined to support c++11 by default. CMake option ENABLE_CUDA is used turn on/off CUDA.
• Point cloud exporter should work properly now.
• Naming convention on files.

And here are things on the TODO list:

• User has reported bugs while using different projector resolutions. This issue has currently addressed in CPU implementation, but not yet in GPU.
• Unit tests.
• Documentation on the key functions and APIs.
• Refactoring, need more sensible camera and reconstructor structure.
• Update readme accordingly.

from joss-reviews.

@v3c70r great. That is fine. Thanks for the update.

from joss-reviews.

@v3c70r any more updates on progress? We'll give you the time you need but it would be good to keep the review process going.

from joss-reviews.

Sorry for the late reply.
In the latest update, a test has been added and a continuous integration is set up using TravisCI. The test is CPU only for now.

from joss-reviews.

Sorry for so much whining about the build process, but I really think we should have a standard of "it just works, or at least there are very explicit instructions". I haven't tried working though this, but below what I'm now getting. Did you intend to provide Findglm.cmake ?

CMake Error at CMakeLists.txt:29 (find_package):
By not providing "Findglm.cmake" in CMAKE_MODULE_PATH this project has
asked CMake to find a package configuration file provided by "glm", but
CMake did not find one.

Could not find a package configuration file provided by "glm" with any of
the following names:

glmConfig.cmake
glm-config.cmake

Note that I have libglm-dev installed:

doria@doria-home:~/source/3DUNDERWORLD-SLS-GPU_CPU$ sudo apt install libglm-dev
Reading package lists... Done
Building dependency tree
Reading state information... Done
libglm-dev is already the newest version.
The following packages were automatically installed and are no longer required:
dkms libgsoap4 libntdb1 python-ntdb
Use 'apt-get autoremove' to remove them.
0 upgraded, 0 newly installed, 0 to remove and 64 not upgraded.

from joss-reviews.

I installed glm from source code and it can be found. It looks like that the libglm-dev package doesn't have the config file. I will provide a findglm.cmake to cover this situation.

from joss-reviews.

@v3c70r any updates to report? Let me know when the review process can be resumed.

from joss-reviews.

Yes, most of the bugs has been fixed. Both GPU and CPU reconstructions works for two test data sets. One issue left is that the colors are not yet correct in ply files and I'm working on it. Also the test data has been moved to a submodule.

from joss-reviews.

I'm not sure I understand the question "Version: Does the release version given match the GitHub release (v4.0.0)?" - there are tons of changes since this release (7 months ago) - shouldn't we just be concerned with whether the branch that is approved here is tagged/referenceable?

from joss-reviews.

The test doesn't seem to look for data in the correct place (see ctest output below). Presumably it's from these lines in RunTest.cpp:

const std::string L_IMGS = "../../data/arch/leftCam/dataset1/";

That should probably be something more like:

const std::string L_IMGS = dataPath + "/data/arch/leftCam/dataset1/";

In fact, I'd rather see ctest call the SLS executable the same way a user would from the command line. That is, instead of a special .cpp file (RunTest.cpp), the test would not even need its own add_executable but would just look like:

add_test( NAME CPU_TEST
COMMAND [path/to/executable/]SLS [options])

It also doesn't make sense to me that the SLS executable is also looking for data in some particular place (see also below). It seems like the path to the data should be a required parameter.

doria@doria-home:~/build/underworld$ ./bin/SLS
No image read from ../../data/alexander/rightCam/dataset1/ ... No image read from ../../data/alexander/leftCam/dataset1/ ... Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Failed to load config ../../data/alexander/rightCam/calib/output/calib.xml
Segmentation fault (core dumped)

doria@doria-home:~/build/underworld$ ctest -V
UpdateCTestConfiguration from :/home/doria/build/underworld/DartConfiguration.tcl
UpdateCTestConfiguration from :/home/doria/build/underworld/DartConfiguration.tcl
Test project /home/doria/build/underworld
Constructing a list of tests
Done constructing a list of tests
Updating test list for fixtures
Added 0 tests to meet fixture requirements
Checking test dependency graph...
Checking test dependency graph end
test 1
Start 1: CPU_TEST

1: Test command: /home/doria/build/underworld/test/runCPUTest
1: Test timeout computed to be: 9.99988e+06
1: Running main() from gtest_main.cc
1: [==========] Running 2 tests from 1 test case.
1: [----------] Global test environment set-up.
1: [----------] 2 tests from RunCPUTest
1: [ RUN ] RunCPUTest.Arch
1: No image read from ../../data/arch/rightCam/dataset1/ ... Failed to load config ../../data/arch/rightCam/calib/output/calib.xml
1: Failed to load config ../../data/arch/rightCam/calib/output/calib.xml
1: Failed to load config ../../data/arch/rightCam/calib/output/calib.xml
1: Failed to load config ../../data/arch/rightCam/calib/output/calib.xml
1/1 Test #1: CPU_TEST .........................***Exception: SegFault 0.15 sec

0% tests passed, 1 tests failed out of 1

Total Test time (real) = 0.16 sec

The following tests FAILED:
1 - CPU_TEST (SEGFAULT)
Errors while running CTest

from joss-reviews.

@daviddoria

Thank you for your input,
I have removed the fixed path from default command line parameter.

Regarding the test, since the main contribution of the project are the reconstruction libraries, the main purpose of the test focuses more on the libraries instead of the reconstruction binaries. Also with fixed data path, it would be easier to integrate with the CI.

from joss-reviews.

@v3c70r
Sorry, I'm not buying either of those arguments.

Regarding the test, since the main contribution of the project are the reconstruction libraries, the main purpose of the test focuses more on the libraries instead of the reconstruction binaries.

If you look at App.cpp, it is almost identical to each one of the TEST() in RunTest.cpp . Both very clearly demonstrate the library - there is almost zero overhead in the executable, so I don't know how you think the test would be "testing the binary" instead of "testing the library"?

My point here is avoiding code duplication for maintenance reasons - if you change an API, you will have to update identical code in both App.cpp and RunTest.cpp which just seems unnecessary.

Also with fixed data path, it would be easier to integrate with the CI.

Those fixed paths imply you are using an in-source build, which is almost always bad practice. I also find it hard to believe that your CI doesn't integrate with CMake well enough to pass a path to the test executable.

My main point is that if I'm a user and run 'make; ctest' and see 100% of tests fail, I don't get a warm fuzzy feeling.

from joss-reviews.

@daviddoria
As suggested, I have added two mandatory cmake parameters: TEST_DATA_PATH and TEST_REF_PATH when test is enabled. Now both application and test can be built and run out of source.

My point here is avoiding code duplication for maintenance reasons - if you change an API, you will have to update identical code in both App.cpp and RunTest.cpp which just seems unnecessary.

I agree with your point here, but I would like to stick with the tests on the library since it will be more flexible. I could extend the test to cover more code and add tests to test only part of the code (e.g. tests on mask ). App.cpp contains few code and update it won't be too much trouble (though not quit elegant).

from joss-reviews.

I agree with your point here, but I would like to stick with the tests on the library since it will be more flexible. I could extend the test to cover more code and add tests to test only part of the code (e.g. tests on mask ). App.cpp contains few code and update it won't be too much trouble (though not quit elegant).

This seems reasonable to me. We definitely don't want 'perfect to be the enemy of good' here and the fact that there are any tests puts this software head and shoulders above much of the scientific software online.

@daviddoria - your feedback & input here is much appreciated. In your opinion how close are we to finishing up this review?

from joss-reviews.

"We definitely don't want 'perfect to be the enemy of good' here"

Totally agree. I thought this would be an easy fix since it was just de-duplication :)

"and the fact that there are any tests puts this software head and shoulders above much of the scientific software online."

I would philosophically disagree on this one! Just because everyone else is making crap doesn't mean we should lower the bar! "If it's not tested, it's broken" is a pretty solid motto to live by in my opinion.

In the instructions, this:
git clone https://github.com/theICTlab/3DUNDERWORLD-SLS-GPU_CPU.git

needs to be changed to
git clone --recursive https://github.com/theICTlab/3DUNDERWORLD-SLS-GPU_CPU.git

since the data is now a submodule. You should assume most users will see an empty folder and be very confused (and not know "ah, I should just 'git submodule init; git submodule update' ")

Guys, can we please try to look at this from a new user's point of view? Why not auto-set TEST_DATA_PATH to ${CMAKE_SOURCE_DIR}/data/whatever ? I don't know the difference between "TEST_DATA_PATH" and "TEST_REF_PATH", and I've been looking at this for a while now. These are not explained in the CMake, and there are no "obvious" folders that one could match to this without instructions (i.e. if there was data/alexander/test and data/alexander/ref)

Unfortunately I'm sensing everyone is starting to get antsy to accept this, while I have barely even started to review the actual code at all! I never even looked in any of the .cpp files, because most of the .h files didn't give me a clue what was going on/what I should be looking for. Many of my comments (particularly about uncommented interfaces) from Oct. 8 are still unaddressed.

I'm happy to step down as a reviewer if you're looking for an "easier" review/reviewer, but I've spent a decade being frustrated with exactly these sorts of issues, so I'm not going to sign off on something that contains them!

from joss-reviews.

As the reviewer guidelines http://joss.theoj.org/about#reviewer_guidelines say

Reviewers are expected to install the software they are reviewing and to verify the core functionality of the software.

Our guidelines/requirements in relation to tests are at present rather meagre, as it simply says:

Tests
Authors are strongly encouraged to include an automated test suite covering the core functionality of their software.
Good: An automated test suite hooked up to an external service such as Travis-CI or similar
OK: Documented manual steps that can be followed to check the expected functionality of the software (e.g. a sample input file to assert behaviour)
Bad (not acceptable): No way for you the reviewer to check whether the software works

Therefore in relation to testing, a simple or sub-optimal tests system may suffice. Having said that, obviously the reviewer should be able to actually set-up the software environment, and it should, through the tests implemented, by easy to assess the core functionality of the software. If the tests are so poorly implemented the latter is not feasible we need to recommend to revise the test suite. If instead the tests are just sub-optimal we may recommend revision but acceptance may in this case (if the core functionality can be evaluated appropriately) not depend on this.

I feel @daviddoria has done a marvellous job at reviewing and I have been very impressed with the speed and level of detail and commitment of his review work. I can assure you we are not “antsy to accept” at this point. From all of the reviewer's comments it seems clear that even as an expert there have been many challenges to overcome to finally get to the point where we can start to evaluate the core functionality of this submission.

@daviddoria thanks again for your efforts on this. This submission is clearly benefiting from the issues you have have raised. You mention that some of the issues raised e.g. around October 8th have not been addressed yet. To avoid confusion, and if it is not too much trouble, would you be so kind to provide an updated list of points that remain to be addressed. Keeping in mind the reviewer guidelines and our current “not so strict” rules on testing methods.

@v3c70r please address all of the points raised by the reviewers. If you disagree on certain aspects clearly detail your arguments against the issues raised and refer to our submission guidelines where possible.

@mikhail-matrosov and @vaheta thanks also to your review contributions. If you are available, it would be much appreciated if you could also pick up the review process as it seems we should now be able to start to get at the core functionality of the software.

from joss-reviews.

Guys, can we please try to look at this from a new user's point of view? Why not auto-set TEST_DATA_PATH to ${CMAKE_SOURCE_DIR}/data/whatever ? I don't know the difference between "TEST_DATA_PATH" and "TEST_REF_PATH", and I've been looking at this for a while now. These are not explained in the CMake, and there are no "obvious" folders that one could match to this without instructions (i.e. if there was data/alexander/test and data/alexander/ref)

I have removed those two unnecessary parameters and use ${CMAKE_SOURCE_DIR} instead.

Regarding the comment earlier

From looking at Reconstructor.h, I don't understand the API. What does it mean to add a camera to this object? It's not clear that pointCloud_ (I think) is an output object. I don't know what a Projector is. what is idx_? Why does reconstruct() not return the point cloud?

Now the reconstruct() function returns a point cloud and the point cloud can be exported to ply or obj file.

There are still some documentation works to be done but the core function should be working fine now.

I'd like to thank @daviddoria and everybody here who gave me very professional and useful comments, which makes the software more useful and allows me to write better code in the future.

from joss-reviews.

Excellent - 'make' now works out of the box, and 'ctest' runs with no setup required and the test passes!

Some more notes (in no particular order):

• everything in test/splitstring should be able to be replaced with a couple of calls to some stdlib functions, no? (or using std::stringstream or something). It's super "old school" as-is.

• I really like how "thin" the files in app/ are!

• In Calibrator.hpp:

  • showImgAvecText_Block - is that Avec=French for "with"? I'd suggest making all function names in English
  • most of the functions do not have descriptions - particularly important is Calibrate()
  • It seems like the UI for selecting the checkerboard corners should be separated from the Calibrate() function, as one could imagine providing their own pixels from another process instead of using this UI

• It seems odd that FileReader derives from Camera? (and following that, that it has functions like undistortPixel, etc.)

• I would provide an explanation in PointCloud.hpp of why you are using a giant float array with all of the values from all of the points instead of the expected vector or similar

• The documentation for Projector doesn't explain what it does, and presumably it's a core component? (though it doesn't seem to do really anything related to "projection"?)

• GrayCode.hpp has zero documentation

• I would add some "why do we need this" motivation to DynamicBitset. I wonder if it's worth a Boost dependency for boost::dynamic_bitset? It seems like a lot of highly error-prone code in there.

• In ReconstructorCPU.h:

  • What are "buckets"? They seem to be a core concept, but are not explained at all
  • If you could break the two big functions down into 3 or 4 each, I imagine the code would be much easier to follow. Good places to start are with the loops - whatever is happening each time through the loop is typically a very easy thing to extract and name. Then by looking at the class interface, the reader can know all of the things that happen, and by looking at the implementation they just learn the order in which those things happen. It also allows a much more "hey, I could change/customize this to try out my new idea!" attitude, instead of "gah, this function is huge, I'll probably break something if I touch it"

• In the README, I would replace the copy/pasted code (#1: if it's not compiled, it's broken even more than if it's not tested, and #2, you should just be able to point the reader to one of the real files that exists already) with a high level system description. That is, something like (but hopefully more involved) "The implementation is very straight forward. We construct a collection of Camera objects, and use the Reconstructor to them to do XYZ with Buckets (which definitely seems like it wants to be a real object) which finally produces a PointCloud of the reconstruction".

from joss-reviews.

Thanks @daviddoria , I'm starting to work on the issues now.

I would add some "why do we need this" motivation to DynamicBitset. I wonder if it's worth a Boost dependency for boost::dynamic_bitset? It seems like a lot of highly error-prone code in there.

I take a quick look at the boost::dynamic_bitset. It looks like exactly the library I want.

The documentation for Projector doesn't explain what it does, and presumably it's a core component? (though it doesn't seem to do really anything related to "projection"?)

The projector is literally a projector that projects the pattern on the object for reconstruction. I'll add more details in the documentation.

It seems odd that FileReader derives from Camera? (and following that, that it has functions like undistortPixel, etc.)

Here the camera is defined as a reconstruction input device. It encapsulates image acquisition, loading configuration, undistortion, and masking out invalid pixels. FileReader acquires images and configurations from file, which satisfies these interfaces. I'm thinking this class probably need a better name like DataInput?

I would provide an explanation in PointCloud.hpp of why you are using a giant float array with all of the values from all of the points instead of the expected vector or similar

I think when deal with the GPU implementation, it would be easier to pass in primitive data types.

from joss-reviews.

Here the camera is defined as a reconstruction input device. It encapsulates image acquisition, loading configuration, undistortion, and masking out invalid pixels. FileReader acquires images and configurations from file, which satisfies these interfaces. I'm thinking this class probably need a better name like DataInput?

I guess the point was that use of a hierarchy seems odd. I'd expect something more like a FileReader/DataInput object to take a file/folder and return a Camera. Having FileReader derive from camera doesn't seem like the right model.

I guess I'm also confused that the class that loads data has a function called 'undistortPixel', unless you're considering part of loading data also "preparing it" in the sense that you're removing distortion from the images you load. This part can be fixed with a few sentences in a class description :)

The getRay functions, computeShadows.., etc. also seem like they should just be directly part of the Camera class.

I think when deal with the GPU implementation, it would be easier to pass in primitive data types.

I assumed that was the case, but it should still be explained in the class to avoid confusion.

One new thing - the variable "color_" in Camera seems like it should actually be called "litImage_" or similar? I hope you can see I'm not trying to be obnoxious and "perfect" names (taking them from "pretty good" to "pretty good++"), but rather trying to take them from "non-descriptive/very confusing" to "readable/comprehensible".

from joss-reviews.

from joss-reviews.

@charalambos I'm still addressing the issues. Here is the progress

from joss-reviews.

@Kevin-Mattheus-Moerman Hi Kevin,
Sorry for tardy reply. I'll be able to review the project again only in about 3 weeks. Is that ok?

from joss-reviews.

@vaheta Thanks. That is fine. It may take the authors some time to work on some of the issues discussed above so when you join in you will hopefully be able to work with the revised/updated version.

from joss-reviews.

@v3c70r I noticed that on your list here https://github.com/theICTlab/3DUNDERWORLD-SLS-GPU_CPU/projects/2 several points are labelled as done. Let us know when the review process can be resumed.

from joss-reviews.

Yes, it is almost done except some polishing. I think we can continue review and I work on both issue fixing and polishing.

from joss-reviews.

Hi guys,

• Great, the build+test now works out of the box!

• The ply files produced still crash Paraview 5.2 (though it can open the obj files properly).

• I get "DOI not found" for this: http://dx.doi.org/10.21105/joss.00047 which appears in the pdf

• I would make the link to this paper https://arxiv.org/pdf/1406.6595.pdf VERY VERY PROMINENT. Without it, it's nearly impossible to figure out what is going on. I would say right at the very top "this is the source code for the structured light implementation described in detail here: ..."

• In general, I'd still like to see better separation of the components for reusability purposes. For example, in GrayCode.*, the generation of the gray code images is done in the same function that displays them. If I wanted to take this code and do something else with the images (write them to a file, etc.), then I'd have to tear it apart. This also is critically important to write testable code.

Going down the list of things to review, I'm confused about the difference between paper.md and README.md. They both seem to be trying to say the same thing, but paper.md is essentially a weird subset of paper.md. Can we have just one (the larger one). The pdf that is generated is mighty sparse to call a "software paper". I would have like to see something like a class diagram or at least a description of the major system components and a description of the workflow (something like "use X to load data, then construct a Y that produces a Z").

"A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?"

No - I don't see anything about why this is necessary (are there no other open source structured light scanners?). "Who is the audience?" is also not addressed - is this for people that want a implementation to build extensions upon? To actually use to create models in production? To use for "research purposes" (read: it probably doesn't work that well), etc. Is it the GPU implementation that makes this new/interesting?

Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?

Yes, but especially since there is only one it should be explained. That is, "run 'make test' and you will see the system transform the "Arch" and "Alexander" datasets from a collection of images into a point cloud. You will find the output in [build]/test/alexander.[ply|obj] and [build]/test/arch.[ply|obj]."

"Community guidelines: Are there clear guidelines for third parties wishing to 1) Contribute to the software 2) Report issues or problems with the software 3) Seek support"

I don't see any of these things listed. Presumably since it's on github I would imagine pull requests are welcome. Should we email the authors (I don't see any email addresses) or create an issue on github if we have a question?, etc.

"A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?"

No - same as above. I'm still not sure why this is asked again here (which is probably what led to the "duplicate" documentation/file).

"References: Do all archival references that should have a DOI list one (e.g. papers, datasets, software)?"

There are no references. I would really like to see some pointers to the "canonical" structured light paper(s) and any existing implementations.

More comments from another pass through the code:

• All 3 files in app/ do not have sufficient explanation. For example I ran 'generateGraycode' and was greeted with a white window... after waiting a while I pressed 'q' to try to quit and then the window turned black. I pressed it again and it split black/white. I then realized it was stepping through graycode targets. This should be explained so I don't have accidentally figure out what to do.
  For CalibrateCamera.cpp - it should be explained what kind of files are expected in the 'images' directory (pairs? numbers required? etc.) and what kind of calibration technique is used/how the parameters are stored in 'output'. After continuing to look, it could at least say "see Calibrator.hpp for input/output requirements/details."

• I saw using boost::dynamic_bitset is in "not now" on your todo list, which is fine, but then DynamicBitset.h needs at least a short explanation of what it is/does (perhaps with a pointer to "if you're familiar with boost::dynamic_bitset, this does approximately the same thing"

• The FileReader class still doesn't have an explanation. This is particularly confusing because it seems to do much more than read files (especially since it derives from Camera). Also, things like 'getParams()' having documentation of "get parameters" is not helpful at all, especially when the 'params_' member has no explanation.

• Log has no explanation. I see a function like 'progress; // display progress bar' and wonder if going to do some UI sort of thing, CLI ####... type of progress indication, etc.

Thanks,

David

from joss-reviews.

• I get "DOI not found" for this: http://dx.doi.org/10.21105/joss.00047 which appears in the pdf

@daviddoria - don't worry about that for now. This is the DOI that the publication will eventually have.

from joss-reviews.

@daviddoria Thanks for your hard work!

About readme.md and paper.md. The paper.md is for JOSS and forms the paper document. The paper itself can be quite minimal and might have some overlap with the readme but should reference works properly though.
See here for guidelines on what it should contain: http://joss.theoj.org/about#author_guidelines

from joss-reviews.

Friendly reminder @charalambos - have you made any progress here?

from joss-reviews.

@v3c70r @charalambos Can you provide an update on progress and if the review process can be resumed? Thanks

from joss-reviews.