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

Allow reconstructions to be extended #178

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

BrookRoberts
Copy link

Not necessarily final, but would like to show the direction I've gone with this before going any further, so looking for comments @paulinus

These changes as a whole should allow you to run opensfm to generate a reconstruction, then place a few new images in the images folder, and then run the opensfm commands again - it will add the new images to the reconstruction without recalculating everything.

Mostly we just check if things are already calculated (e.g. features) and don't redo. For matches, we only calculate matches for the new images, and save them all in the folders of the new images (previously they would go in the image that had the first name alphabetically). For create_tracks we save the unionfind structure, as well as track_ids, which allows us to use previous reconstructions with updated tracks, since we have a persistent way of naming the tracks.

From a test on a large dataset:
First run generating reconstruction of 922 images:
focal_from_exif: 13.5280120373
detect_features: 344.613101006
match_features: 4669.25260997
create_tracks: 129.082221985
reconstruct: 4923.34434319
mesh: 77.7916591167

Re run with no extra images:
focal_from_exif: 0.0260539054871
detect_features: 0.034029006958
match_features: 0.200579881668
create_tracks: 97.3890349865
reconstruct: 188.215790033
mesh: 79.0046401024

Re run with 15 extra images:
focal_from_exif: 0.240912914276
detect_features: 8.93757390976
match_features: 121.738699913
create_tracks: 104.183819056
reconstruct: 442.458048105
mesh: 81.0022370815

(incidentially, this was done on an old set I had used, and it looked like it ran a lot faster than last time - has opensfm had any recent changes that make it faster? or did something change on my end?)

There are still some bits that will be slower with larger reconstructions.

  1. Some of these are because I just haven't taken away the calculations yet: In create_tracks, we convert the unionfind into a graph everytime still.
  2. Some of these are loading times - in adding to an existing reconstruction, we load large files into memory. There might be clever things here that could be done, but there might not.
  3. I haven't changed the mesh command - it might be possible to do something here, or maybe you do have to recalculate whole thing after changing the reconstruction - I haven't looked.
  4. Reconstruction bundle step is slower with larger datasets, this is a general problem.

Problems:
It might result in unexpected behaviour, if you change the images/config, and it doesn't take this into account but uses the already saved results. detect_features already did this, so this change may be an improvement as it makes it more obvious that stuff is not being recalculated (and that you should use clean command to do a new build).

Thoughts? If the approach looks vaguely sensible (the way I'm saving things in create_tracks is maybe not very nice at the moment), and you'd be happy for this to go into master, I can tidy things up/make high level improvements. I'd then appreciate someone thinking about the changes/asking questions reasonably carefully to make sure things work as I think they do.

@BrookRoberts BrookRoberts changed the title Create tracks improvement Allow reconstructions to be extended May 24, 2017
@BrookRoberts
Copy link
Author

#136 is related to this

@paulinus
Copy link
Member

That is really great @BrookRoberts ! i need some time to parse it.

The tricky part is updating the tracks graph. I wonder if it is possible to do without storing the unionfind structure. The information is already in the tracks.csv file although not in a different form. I'm thinking that the structure can be recreated by linking features that belong to the same track, but it seems hard to keep the same track ids.

@BrookRoberts
Copy link
Author

No problem - I'm happy to answer any questions if anything isn't obvious.

Yes, saving the unionfind structure is a bit irritating. It is always going to be small in size compared to the size of the images I think, so I don't think it will add much space/time issues, but it's yucky from a clean code point of view. It seemed to me that it was likely to be less efficient to convent tracks.csv back to unionfind than just saving and loading.

I don't have a good idea of how efficient the unionfind structure is, but I've currently just assumed that the two step of creating unionfind then converting to tracks is good for efficiency.

@paulinus paulinus mentioned this pull request Jun 15, 2017
@BrookRoberts
Copy link
Author

@paulinus what is the status on merging this in? Are you intending to/thinking about how to do it without storing the unionfind structure? Did I misunderstand and you are waiting on me to make some change to that part?

I ask because it no longer merges in cleanly, due to refactoring in match_features.py. I can fix up the merge conflicts, but don't want to have to maintain a branch and keep fixing conflicts! (and this makes it more likely that my changes won't be so fully tested/will have bugs)

@BrookRoberts
Copy link
Author

Given that this isn't actually a trivial merge, but e.g. in match_features has changed in a way that assumes a bit that everything is being recalculated, can we get this merged in before other changes to these files go in?

If you strongly wanted, we could merge the parts that don't involve unionfind first, since each step is kind of standalone, but it would be a lot easier if all the parts went in. I'm not really willing to rewrite stuff every time something gets refactored/restructured.

@BrookRoberts
Copy link
Author

@paulinus - are you interested in this ever being merged in?

@mluogh
Copy link

mluogh commented Sep 16, 2017

@BrookRoberts does this branch currently work out of the box with the latest version of OpenSFM? It would be really useful for a project I'm working on, thanks!

@BrookRoberts
Copy link
Author

@deltameter - sorry for slow reply. To my knowledge, it does, but can't guarantee it is heavily tested. Would be interested in knowing if it works for you / what issues it has. As above, it does still recalculate somethings it might not need to, but is substantially better than not using it.

@happyfrank
Copy link

Hello @BrookRoberts I have a reconstruction of Spherical images and I want to add one Pinhole image(same scene captured) to the scene. Do you think that can be achieved?

@facebook-github-bot
Copy link
Contributor

Hi @BrookRoberts!

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants