[REVIEW]: A Structured-Light Scanning Software for Rapid Geometry Acquisition

[whedon]

Submitting author: @charalambos (Charalambos Poullis)
Repository: https://github.com/theICTlab/3DUNDERWORLD-SLS-GPU_CPU
Version: v4.0.0
Editor: @Kevin-Mattheus-Moerman
Reviewer: @daviddoria
Archive: Pending

Status

Status badge code:

HTML: <a href="http://joss.theoj.org/papers/4329bcbc7bba33961a5e749dcacb995b"><img src="http://joss.theoj.org/papers/4329bcbc7bba33961a5e749dcacb995b/status.svg"></a>
Markdown: [![status](http://joss.theoj.org/papers/4329bcbc7bba33961a5e749dcacb995b/status.svg)](http://joss.theoj.org/papers/4329bcbc7bba33961a5e749dcacb995b)

Reviewer questions

Conflict of interest

• 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

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

Functionality

• Installation: Does installation proceed as outlined in the documentation?
• Functionality: Have the functional claims of the software been confirmed?
• Performance: Have any performance claims of the software been confirmed?

Documentation

• A statement of need: Do the authors clearly state what problems the software is designed to solve and who the target audience is?
• Installation instructions: Is there a clearly-stated list of dependencies? Ideally these should be handled with an automated package management solution.
• Example usage: Do the authors include examples of how to use the software (ideally to solve real-world analysis problems).
• Functionality documentation: Is the core functionality of the software documented to a satisfactory level (e.g. API method documentation)?
• Automated tests: Are there automated tests or manual steps described so that the function of the software can be verified?
• 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

Software paper

Paper PDF: 10.21105.joss.00047.pdf

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

[arfon]

/ 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! 🚀

[davclark]

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.

[arfon]

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.

[labarba]

Thanks for jumping in for JOSS, @davclark !!

[davclark]

And thanks for the invite :)

[davclark]

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.

[charalambos]

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.

[Kevin-Mattheus-Moerman]

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?

[v3c70r]

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

[charalambos]

@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

[pjotrp]

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

[v3c70r]

@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.

[pjotrp]

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

[arfon]

@whedon commands

[whedon]

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).

[arfon]

@whedon list editors

[whedon]

Current JOSS editors:

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

[arfon]

@whedon assign @Kevin-Mattheus-Moerman as editor

[whedon]

OK, the editor is @Kevin-Mattheus-Moerman

[Kevin-Mattheus-Moerman]

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.

[charalambos]

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

[Kevin-Mattheus-Moerman]

@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

[daviddoria]

✋ I will review this weekend.

[Kevin-Mattheus-Moerman]

@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)

[Kevin-Mattheus-Moerman]

@daviddoria Thanks! Welcome on board Sir!

[Kevin-Mattheus-Moerman]

@whedon assign @daviddoria as reviewer

[daviddoria]

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".

[charalambos]

Hi all, I guess this conclude the review? How shall we proceed? Best, C.

…

On Fri, 2017-01-13 at 04:37 -0800, David Doria wrote: 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". — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[v3c70r]

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

[vaheta]

@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?

[Kevin-Mattheus-Moerman]

@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.

[Kevin-Mattheus-Moerman]

@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.

[v3c70r]

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

[daviddoria]

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

[arfon]

• 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.

[Kevin-Mattheus-Moerman]

@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

[arfon]

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

[Kevin-Mattheus-Moerman]

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

[v3c70r]

Thank you for reminding me of the update. Sorry I haven't worked to much on the fixes these days because of tight schedule and Hackathon weekend. I will pick it up this week and hopefully it would be done by the weekend. Thank you for your patience.

[Kevin-Mattheus-Moerman]

Great. Looking forward to it.

[Kevin-Mattheus-Moerman]

@v3c70r please take the time to make the changes required. However do you think you are able to provide an update on progress and perhaps a rough estimate of when you think the work will be done? Thanks.

[v3c70r]

Sorry for making you wait.
I'm refactoring the code to change the Camera into ImageProcessor and moved it out of Reconstructor. It has been done on the CPU and I still need to do the same change on GPU code. As well as the documentation. The updated pipeline is stated in the graph of README.

[Kevin-Mattheus-Moerman]

Great. Let us know when you'd like to resume the review process. Thanks.

[Kevin-Mattheus-Moerman]

@v3c70r any updates to report?

[Kevin-Mattheus-Moerman]

Hi @v3c70r how are you getting on? Do you think we can resume the review process soon? Any updates to report? Thanks

[v3c70r]

Hi @Kevin-Mattheus-Moerman. Sorry there's no update since last time. I'm currently working intensively on a project which will last at least for 3 months. Sorry for the inconvenience.

[Kevin-Mattheus-Moerman]

@v3c70r Okay. Thanks for the update. When you are ready to restart let us know. I hope @daviddoria will be able to remain reviewer and pick up at that point.

[Kevin-Mattheus-Moerman]

@v3c70r @charalambos How are you getting on? Can you post an update on progress? Thanks.

[Kevin-Mattheus-Moerman]

@arfon the authors (@charalambos and @v3c70r ) have indictated (by e-mail to me) that they wish to withdraw this submission. Can you process the withdrawal please?

[Kevin-Mattheus-Moerman]

@daviddoria thank you for your time and effort and continued involvement in this review process! Thank you also @vaheta @mikhail-matrosov for your review contributions.

[arfon]

@arfon the authors (@charalambos and @v3c70r ) have indictated (by e-mail to me) that they wish to withdraw this submission. Can you process the withdrawal please?

Done. Thanks for all of your efforts folks.

[daviddoria]

@Kevin-Mattheus-Moerman Can I ask why? You can imagine how this might be pretty frustrating from my end...