Page MenuHome

GSoC 2016 - Multiview Reconstruction
Needs ReviewPublic

Authored by Tianwei Shen (hlzz001) on Sep 1 2016, 7:19 PM.

Diff Detail

Event Timeline

Tianwei Shen (hlzz001) retitled this revision from to GSoC 2016 - Multiview Reconstruction.
Tianwei Shen (hlzz001) updated this object.
Tianwei Shen (hlzz001) set the repository for this revision to rB Blender.
Tianwei Shen (hlzz001) removed rB Blender as the repository for this revision.

Oh wow, that's a huge patch. No wonder it took so long to have even initial review.

Only did some quick inline notes and here is the bigger picture notes:

Libmv Side

Solver

Do you have explanation somewhere how's the solver is intended to behave now for the multi-camera case? Does it resect/intesect/bundle all cameras at the current frame at once? What if camera does not exist on the current frame?

Biggest thing i'm not sure here is how you managed to use GRIC to find out keyframes for reconstruction.

API

I do not think we should expose Correspondence idea to the Libmv. Unlike Blender side where we can not have "big picture" in the same entity due to possibly linked nature of ID blocks, in Libmv adding Correspondence only adds extra unneeded entity. We do have Marker::clip to indicate the clip marker belongs to.

So basically, in the new auto pipeline all tracks from correspoding tracks from Blender side should go to a single track from Libmv side.

Tests

We really really really prefer to cover all the code with tests. You should be able to use them from simple pipeline.

Duplicated code

Is it possible to get rid of some code there since we moved from simple pipeline to a new auto pipeline?

Blender side

General notes

Even tho this patch seems to include the whole branch, don't see much deletions in there. We should be using new auto pipeline for both single-camera and multi-camera reconstruction. That would be a good test to new API as well.

Split the patch

Think it should be possible to split patch into smaller areas, such as:

  • New Libmv API.
  • New Blender's SpaceClip interface with split regions (without hookinh up actual solver logic)
  • Hook up new solver API to Blender.
  • Remove all "legacy" solver business form Libmv and Blender

That would make it easier to review and accept patches which could momnetarely be applied to a temp branch when then is merged to master in one go.

Users testing

Do you have some good non-artificial example of the new feature?

Did you get any user feedback on interface and workflow here?

intern/libmv/intern/reconstructionN.cc
98

Don't mix two operations in a single line.

369

What is exact limitation here?

intern/libmv/intern/reconstructionN.h
46

In Blender and C-API we follow Type *foo NOT Type* foo. Might be different in Libmv itself.

intern/libmv/libmv/autotrack/reconstruction.h
68

This is a google style we follow in Libmv's code to have space prior to the access qualifier.

source/blender/blenkernel/intern/context.c
689

Not sure this is best naming and place here. CTX_ functions should be getting things from a context and not do lookups around or such.

source/blender/editors/space_clip/clip_ops.c
310

This is annoying to have full copy of logic from regular open here.

At least should be moved to some sort of utility function without such a duplicated logic in both operators.

Ideally should find a way to re-use same operator, but guess i'll need to have a look why that was not possible already.

Tianwei Shen (hlzz001) edited edge metadata.
Tianwei Shen (hlzz001) marked 3 inline comments as done.

Merge the latest master branch.

Hi Sergey,

I also felt that the current patch is to big to be merged. I will follow your suggestions and split this patch into several smaller patches that can be not only useful for multi-view camera reconstruction, but also for the automatic feature tracking pipeline. For example, I think removing some duplicated code of simple_pipleine to automatic_pipeline would be a good start. What do you think?