m(o)ve over smvs #921

smathermather · 2018-12-02T17:50:47Z

There are one big limitation: I have replaced smvs with mve, but perhaps it should be optioned instead.

smathermather · 2018-12-03T04:35:47Z

Ok @pierotofy and @dakotabenjamin -- this will require some heavy testing. mve seems to have lower memory requirements but increased processing time over smvs. I think for the additional fidelity, the processing time is worth the tradeoff (especially in light of reduced memory usage, but we should test on bigger datasets to be sure.

And if we need to have them all run side-by-side, I am open to that too. We could have low, medium, and high fidelity 3D data with opensfm, smvs, and mve point clouds respectively. But, for the sake of simplicity, this pull request is an outright replacement of smvs.

Also, I pass no parameters for mve -- the results are quite nice with defaults. We may want to add parameters later, but I decided to keep this simple: given the quality of the improvement, there seemed sense to not over-complicate.

Improve aspect ratio for many users

Literally string literals

pierotofy · 2018-12-04T02:03:44Z

I think less is more, so if quality is improved and memory usage is lowered, even though processing time is increased, I think it's a win and smvs can be placed aside for the time being.

Is it ready to be tested then? I'd love to start running some tests.

pierotofy · 2018-12-04T02:13:50Z

scripts/mve.py


            # config
            config = [
-                "-t%s" % self.params.threads,


We should respect the --max_concurrency parameter when invoking dmrecon?

Ah yes, this reminds me of the other hitch -- I am not sure dmrecon respects concurrency values. It'd have to be set with OMP: https://github.com/simonfuhrmann/mve/blob/master/apps/dmrecon/dmrecon.cc#L285

Interesting, I don't have much experience with OpenMP. Maybe calling dmrecon by setting the OMP_NUM_THREADS environment variable could limit them. https://www.ibm.com/support/knowledgecenter/SSGH2K_13.1.3/com.ibm.xlc1313.aix.doc/compiler_ref/ruomprun4.html

Yes, I tried that and manually set it to 5 but it used all cores anyway... . But it is c++, there's a good chance I was doing something wrong.

This is one I would need help on -- I'm a bit over my head.

Might just be able to port the approach that smvs uses, if their approaches to concurrency are similar enough.

If the user selects mve as the processing option, and also specifies --max-concurrency, then a warning ("Thread limiting is not available for this option and will run on all threads, are you sure you want to continue? [Y/n]

If we can't limit the threads a warning is fine, but I'd caution with the Y/n prompt, could cause nodeodm to get stuck indefinitely.

smathermather · 2018-12-04T20:24:42Z

It is ready to be tested. It should just install and run.

pierotofy · 2018-12-04T22:35:55Z

I will make some time for this in the next few days.

smathermather · 2018-12-05T00:46:19Z

👍

smathermather · 2018-12-05T22:17:43Z

I will make some time for this in the next few days.

I am pretty sure I did something wrong here. It usually works as I have it configured, but sometimes failed (I'm sorry I can't be more specific).:

ODM/scripts/odm_app.py

Line 121 in 434b810

# run mve

pierotofy · 2018-12-07T13:39:36Z

Testing in progress, will post updates when I get results.

pierotofy · 2018-12-10T15:40:10Z

My findings so far:

Sharp edges are much better preserved compared to SMVS, which creates better DSMs.
Noise levels are elevated, in particular there's a consistent pattern of points being "raised" from the contours of objects.

The number of points generated is often overkill, there's probably a flag to use a lower resolution depthmap and we should take advantage of it.
The output from dmrecon uses special characters to display the progress bar, if I recall correctly there's a flag to use more conventional / silent progress output without special characters. We should use that as well.

I'll look for flags that could help us solve one or more of these problems.

pierotofy · 2018-12-10T15:45:40Z

Here's another example:

pierotofy · 2018-12-10T17:00:17Z

The flag we should always pass: --progress=silent. It defaults to fancy.

Should also pass --force for good measure.

--max-pixels uses the same logic as smvs, (the code is the same, so perhaps you can re-add it?). This should reduce both memory usage, speed and perhaps noise.

It is rarely useful to reconstruct at full resolution as it will produce less complete depth maps with more noise at a highly increased processing time.

https://github.com/simonfuhrmann/mve/wiki/MVE-Users-Guide#multi-view-stereo

All options for reference:

Available options: 
  -n, --neighbors=ARG    amount of neighbor views (global view selection)
  -m, --master-view=ARG  reconstructs given master view ID only
  -l, --list-view=ARG    reconstructs given view IDs (given as string "0-10")
  -s, --scale=ARG        reconstruction on given scale, 0 is original
  --max-pixels=ARG       Limit master image size [1500000]
  -f, --filter-width=ARG  patch size for NCC based comparison [5]
  --nocolorscale         turn off color scale
  -i, --image=ARG        specify source image embedding [undistorted]
  --local-neighbors=ARG  amount of neighbors for local view selection [4]
  --keep-dz              store dz map into view
  --keep-conf            store confidence map into view
  -p, --writeply         use this option to write the ply file
  --plydest=ARG          path suffix appended to scene dir to write ply files
  --bounding-box=ARG     Six comma separated values used as AABB [disabled]
  --progress=ARG         progress output style: 'silent', 'simple' or 'fancy'
  --force                Reconstruct and overwrite existing depthmaps

pierotofy · 2018-12-10T20:18:50Z

opendm/context.py

 makescene_path = os.path.join(superbuild_path, 'src', 'elibs', 'mve', 'apps', 'makescene', 'makescene') #TODO: don't install in source
-smvs_path = os.path.join(superbuild_path, 'src', 'elibs', 'smvs', 'app', 'smvsrecon')
+mve_path = os.path.join(superbuild_path, 'src', 'elibs', 'mve', 'apps', 'dmrecon', 'dmrecon')
+mve_path_pc = os.path.join(superbuild_path, 'src', 'elibs', 'mve', 'apps', 'scene2pset', 'scene2pset -F2')


-F2 should not be hard coded as it references the chosen scale value (depends on --max-pixels). If a dataset has really large input images (or really small images), this will fail. I think the value can be looked up by looking at the contents of the .mve folders.

Another alternative is to calculate a scale value ourselves instead of letting MVE sort it out and pass it to both scene2pset (via -F) and to dmrecon (via -s).

Maybe this is why you noticed a few failures during your tests.

pierotofy · 2018-12-10T20:31:01Z

Reducing the scale of reconstruction reduces the noise a bit, but perhaps this is just the result of having fewer points overall?

pierotofy · 2018-12-10T20:52:58Z

As far as scene2pset, I think we'll want to leverage the scale and confidence values to do the filtering. In particular, I think discarding points below a certain confidence value is a must. I'm getting some good results by discarding values below 0.25 confidence. Perhaps there's a better value, or maybe it should be user-defined.

pierotofy · 2018-12-10T21:05:28Z

scene2pset parameters:

Usage: scene2pset [ OPTS ] SCENE_DIR MESH_OUT
Available options: 
  -d, --depthmap=ARG       Name of depth map to use [depth-L0]
  -i, --image=ARG          Name of color image to use [undistorted]
  -n, --with-normals       Write points with normals (PLY only)
  -s, --with-scale         Write points with scale values (PLY only)
  -c, --with-conf          Write points with confidence (PLY only)
  -m, --mask=ARG           Name of mask/silhouette image to clip 3D points []
  -v, --views=ARG          View IDs to use for reconstruction [all]
  -b, --bounding-box=ARG   Six comma separated values used as AABB.
  -f, --min-fraction=ARG   Minimum fraction of valid depth values [0.0]
  -p, --poisson-normals    Scale normals according to confidence
  -S, --scale-factor=ARG   Factor for computing scale values [2.5]
  -C, --correspondence     Output correspondences (in absence of -m and -b only)
  -F, --fssr=ARG           FSSR output, sets -nsc and -di with scale ARG

To export confidence values, pass -c.

pierotofy · 2018-12-10T21:20:29Z

Overall I think things that could be added/improved:

Pass scale args to both dmrecon and scene2pset.
Filter based on confidence level. This can be done via PDAL.
Fix progress flag.

Finding how to respect --max-concurrency is also a priority, but the PR could be merged without it.

smathermather · 2018-12-11T16:28:14Z

Cool. I’ll dive in. This is an awesome review.

pierotofy · 2019-04-10T19:14:42Z

DEMs improved!

pierotofy · 2019-04-10T19:18:34Z

Getting some bad distortion on certain datasets however.

pierotofy · 2019-04-10T19:27:13Z

Probably due to noise:

pierotofy · 2019-04-11T01:09:18Z

0.6 as a default confidence threshold yields much better results:

pierotofy · 2019-04-12T17:47:15Z

Fixes #970

pierotofy · 2019-04-12T17:59:39Z

max-concurrency is now respected on makescene, dmrecon, scene2pset.

@smathermather @indiajohnson-cm this can be tested and pending further issues I think it can be merged into master as-is.

pierotofy · 2019-04-13T21:54:12Z

Investigating a seg fault for one test dataset (sheffield_cross) that processed fine with SMVS.

pierotofy

Seg fault did not happen again after more testing. This might be tied to some sort of racing condition, but for the time being I think it's OK to move forward.

@smathermather OK to increase version number and merge in your opinion?

pierotofy · 2019-04-20T14:57:12Z

Implemented #972

smathermather · 2019-04-22T14:13:09Z

Seg fault did not happen again after more testing. This might be tied to some sort of racing condition, but for the time being I think it's OK to move forward.

@smathermather OK to increase version number and merge in your opinion?

Yes to both! Tests are looking good on my end.

smathermather · 2019-04-22T14:15:23Z

0.6 as a default confidence threshold yields much better results:

This matches what I was seeing too -- confidence thresholds greater than 0.5 needed for best results.

smvs added back in by Piero

smathermather · 2019-04-22T14:25:34Z

Hmm, I dismissed @dakotabenjamin's review based on a myth I think... . @pierotofy -- smvs is still gone from this pull request, correct?

pierotofy · 2019-04-22T14:43:05Z

Yes SMVS is gone.

smathermather · 2019-04-22T15:49:27Z

A sibling docs update that I meant to do as a pull request but did on the active repo (yipe!):
OpenDroneMap/docs#13

Anyway, docs should reflect these changes and more. Let me know if I missed anything.

m(o)ve over smvs Former-commit-id: 57ca7d9

mve over smvs

8e3a01f

smathermather requested review from dakotabenjamin and pierotofy December 2, 2018 17:50

Stephen Mather added 5 commits December 2, 2018 18:20

fix misnamed function

42c140d

fix mve process. Still need to generate point cloud

fd2dd61

added point cloud creation, need parameters

7f21215

finish prospective mve changes

91a6616

full removal of smvs

ae3221a

Stephen Mather and others added 3 commits December 3, 2018 04:38

remove parameters from mve.py

9f8d10a

Merge pull request #3 from smathermather/master

16ecdfa

Improve aspect ratio for many users

Merge pull request #4 from smathermather/master

434b810

Literally string literals

pierotofy reviewed Dec 4, 2018

View reviewed changes

smathermather mentioned this pull request Dec 7, 2018

odm_georef fails with large point clouds (exit code 139, seg fault) #909

Closed

smathermather mentioned this pull request Dec 7, 2018

Exporting Projection Matrix #919

Closed

pierotofy reviewed Dec 10, 2018

View reviewed changes

smathermather requested a review from dakotabenjamin April 4, 2019 16:42

pierotofy added 2 commits April 10, 2019 10:16

Merged master

5261c27

Better scale calculation, defaults

9561b67

Change default confidence to 0.6

a41f0c6

pierotofy added 2 commits April 11, 2019 16:29

Tiled DEM generation (WIP)

4fb27d6

Multithread DEM tiling, more memory tollerant, better DEM gapfill

460e263

Respect max-concurrency in makescene, dmrecon, scene2pset

9c3e635

pierotofy self-requested a review April 12, 2019 18:00

pierotofy approved these changes Apr 19, 2019

View reviewed changes

pierotofy added 2 commits April 20, 2019 10:33

Updated version number

fa3008c

Exposed SMRF parameters

2b74a03

smathermather mentioned this pull request Apr 22, 2019

Update docs to reflect API / flag changes OpenDroneMap/docs#13

Closed

pierotofy merged commit 57ca7d9 into OpenDroneMap:master Apr 22, 2019

smathermather deleted the mvs_and_smvs branch April 23, 2019 00:33

peddyhh pushed a commit to peddyhh/ODM that referenced this pull request Jun 7, 2020

Merge pull request OpenDroneMap#921 from smathermather/mvs_and_smvs

08af094

m(o)ve over smvs Former-commit-id: 57ca7d9

pierotofy added a commit that referenced this pull request Sep 21, 2020

Merge pull request #921 from smathermather/mvs_and_smvs

8165697

m(o)ve over smvs Former-commit-id: 57ca7d9

m(o)ve over smvs #921

m(o)ve over smvs #921

Conversation

smathermather commented Dec 2, 2018 • edited

smathermather commented Dec 3, 2018 • edited

pierotofy commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smathermather commented Dec 4, 2018

pierotofy commented Dec 4, 2018

smathermather commented Dec 5, 2018

smathermather commented Dec 5, 2018

pierotofy commented Dec 7, 2018

pierotofy commented Dec 10, 2018

pierotofy commented Dec 10, 2018

pierotofy commented Dec 10, 2018 • edited

pierotofy Dec 10, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierotofy commented Dec 10, 2018

pierotofy commented Dec 10, 2018 • edited

pierotofy commented Dec 10, 2018

pierotofy commented Dec 10, 2018 • edited

smathermather commented Dec 11, 2018

pierotofy commented Apr 10, 2019 • edited

pierotofy commented Apr 10, 2019

pierotofy commented Apr 10, 2019

pierotofy commented Apr 11, 2019

pierotofy commented Apr 12, 2019

pierotofy commented Apr 12, 2019

pierotofy commented Apr 13, 2019

pierotofy left a comment • edited

Choose a reason for hiding this comment

pierotofy commented Apr 20, 2019

smathermather commented Apr 22, 2019

smathermather commented Apr 22, 2019

smathermather commented Apr 22, 2019

pierotofy commented Apr 22, 2019

smathermather commented Apr 22, 2019

smathermather commented Dec 2, 2018 •

edited

smathermather commented Dec 3, 2018 •

edited

pierotofy commented Dec 10, 2018 •

edited

pierotofy Dec 10, 2018 •

edited

pierotofy commented Dec 10, 2018 •

edited

pierotofy commented Dec 10, 2018 •

edited

pierotofy commented Apr 10, 2019 •

edited

pierotofy left a comment •

edited