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

cmakelists.txt missing find_package(zlib REQUIRED) #10

Open
tksharpless opened this issue Sep 29, 2019 · 36 comments
Open

cmakelists.txt missing find_package(zlib REQUIRED) #10

tksharpless opened this issue Sep 29, 2019 · 36 comments

Comments

@tksharpless
Copy link

No description provided.

@tksharpless
Copy link
Author

Hey I f*ing well did provide a nice long description with steps to reproduce, and GitHub ate them. e-mail me if you want that. Meanwhile, just put find_package(zlib REQIRED) at line 48 of cmakelists.txt.

@tksharpless
Copy link
Author

Two more lines in cmakelists.txt contain gcc-isms that give msvcc commandline errors:
36 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++11 -Wall -Wsign-compare")
76 set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Werror")
in msvcc -W expects a numeric argument, but cmake copies the strings.

However this is no problem compared to the fact that Folly is rife with code that will not compile on vc 14. Suggestions welcome.

@aparrapo
Copy link
Contributor

Hi @tksharpless - The CMakeLists.txt file at the top level has only been tested on Mac and Linux. We use Docker on Windows machines. If can make it work on Windows that's awesome, but please make sure that it doesn't break for other platforms (haven't tried myself). Then you can open a pull request and we can give you proper attribution :)

Regarding Folly for Windows, have you checked the steps in https://facebook.github.io/facebook360_dep/docs/rift? It may not be what you want but worth trying.

@tksharpless
Copy link
Author

tksharpless commented Oct 1, 2019 via email

@aparrapo
Copy link
Contributor

aparrapo commented Oct 1, 2019

Hi @tksharpless - we built the pipeline with cases like yours in mind. The calibration stage can definitely be parsed out and run independently, as shown in https://facebook.github.io/facebook360_dep/docs/calibration

As for Folly, we mostly use it to parse json files and print log statements, e.g.
https://github.com/facebook/facebook360_dep/blob/master/source/calibration/GeometricCalibration.cpp

so you can probably replace the calls with standard std::cout and use a different json parser.

If you have any issues removing dependencies let us know and we'll try to make it work :)

@tksharpless
Copy link
Author

tksharpless commented Oct 2, 2019 via email

@aparrapo
Copy link
Contributor

aparrapo commented Oct 2, 2019

Our Camera class supports f-theta, rectilinear, equisolid and orthographic models, and can be extended to any model. It's also distortion agnostic, i.e., we model the distortion by fitting a radially symmetric high-order polynomial:

https://github.com/facebook/facebook360_dep/blob/master/source/util/Camera.h

The distortion parameters for most of the existing lenses can be found in third-party software like Photoshop.

We tried using PTGui to get approximate positions for a fairly complex 360 rig and it failed, but it may work with simpler arrangements. Combining these positions with the lens model and distortion parameters from above you should be able to calibrate the rig :)

@tksharpless
Copy link
Author

tksharpless commented Oct 2, 2019 via email

@tksharpless
Copy link
Author

Having better luck compiling with VC 15 (Visual Studio 2017). But found what looks like a real source code issue: the code uses 'filesystem::' and 'boost::filesystem::' as if they were synonyms; but in my vc environment the former resolves to 'std::experimental::filesystem::', a different namespace. I suppose there may be a way to configure around this, let you know if I find one.

@aparrapo
Copy link
Contributor

aparrapo commented Oct 3, 2019

It's actually a feature :)

https://github.com/facebook/facebook360_dep/blob/master/source/util/FilesystemUtil.h

it should just work, but if not you can modify FilesystemUtil.h and it will be applied everywhere.

@tksharpless
Copy link
Author

Yeah, Ifound that

#ifdef WIN32
#include
#else
#define BOOST_FILESYSTEM_NO_DEPRECATED
#include <boost/filesystem.hpp>
#endif

But this will not work because the code has "boost::filesystem::" in many places.
The answer is obviously to disable it and just use boost.
Unfortunately that leaves a couple of error messges about the lack of a conversion from boost::filesystem::path::string_type to std::string. I guess gcc has such a conversion but vc does not. So the proper solution must be to globally replace "boost::filesystem" with "fileystem" and use std on Windows. There are 33 instances scattered over 13 files. No problem to change them.

BUT actually must use boost anyhow. 4 of your headers explicitily include <boost/filesystem.hpp>
Moreover boost::filesystem has a member path that your code references a lot, and std::experimental::filesystem::v1 does not.

So bottom line is use boost::filesystem and provide that string conversion function on Windows.

@tksharpless
Copy link
Author

That leaves just one compile error in calibration lib. The definitinion of _Associated_state in VC's future.h assumes the target class is default-constructible (this is marked with a ToDo) and sad to say your code is passing it things that are not. I don't think I want to try to fix this.
In fact I plan to re-architect the standalone calibrator to not use multithreading at all, but rather multiple parallel processes, which is cleaner and simpler and typically gives a bigger performance boost.

@tksharpless
Copy link
Author

BTW the string conversion problem comes from the fact that boost::filesystem::path is wchar on Windows but char on nixoids.

@tksharpless
Copy link
Author

tksharpless commented Oct 4, 2019 via email

@aparrapo
Copy link
Contributor

aparrapo commented Oct 4, 2019

Thanks good news! Let us know as you build these commands, we can probably help you out.

If your changes diverge too much from the original code you can always fork the repo and apply the changes there:

https://help.github.com/en/articles/fork-a-repo

And in the long run if it's beneficial to the community we can even link to it from here :)

@tksharpless
Copy link
Author

tksharpless commented Oct 4, 2019 via email

@tksharpless
Copy link
Author

tksharpless commented Oct 4, 2019 via email

@tksharpless
Copy link
Author

tksharpless commented Oct 4, 2019 via email

@tksharpless
Copy link
Author

The source of the pathless references to double-conversion.lib is a line in the fb360-dep cmakelists.txt which adds that as a literal name to the link list for Folly. This is not needed here because vcpkg installs double-conversion as a first class package. Updated unified patch attached.
calibrationLibWin32.patch.txt

@aparrapo
Copy link
Contributor

aparrapo commented Oct 7, 2019

Can you open a pull request with those changes? We can then easily test it on Linux and Mac on our end and then give you proper attribution :)

@tksharpless
Copy link
Author

tksharpless commented Oct 7, 2019 via email

@tksharpless
Copy link
Author

The calibration test fails on my 24GB laptop when ceres solver tries to allocate 23GB of RAM. I am looking into ways to make the memory requirement more reasonable. Hopefully just tweaking the solver options will do it; but might have to build with a different sparse matrix library.

@tksharpless
Copy link
Author

Rebuilt ceres with SuiteSparse. Now geometrical calibration demands less that 1GB of RAM.
But it eventually fails after issuing the warning "final pass median error too high". Log attached.
The console displayed one more line that did not make it into the log:

pass 9: RMSEs: Pos 0 Rot 0.00200985 Principal 24.3039 Distortion 0 Focal 4.34938 Angle 0.00132241
Calibration.exe.DELLSIE.tommy.log.INFO.20191007-115430.24328.txt

I await your advice.

@tksharpless
Copy link
Author

Actually the process did not crash. It saves an updated rig file and exits normally. So I guess it can be said to be working. The numbers in this file are mostly similar to the ones in the reference calibrated rig file, but there are some discrepancies on the order of 10%.
I don't know how to evaluate the 'correctness' of this result. Please let me know if you think it is OK.
test-manifold_8192_calibrated.json.txt
reference-manifold_8192_calibrated.json.txt

@tksharpless
Copy link
Author

tksharpless commented Oct 8, 2019 via email

@aparrapo
Copy link
Contributor

aparrapo commented Oct 9, 2019

10% difference is expected with 8k images. The warning is just so you know that the error is pretty high, but a 0.5 pixel error at 8k should be ok.

@tksharpless
Copy link
Author

tksharpless commented Oct 9, 2019 via email

@tksharpless
Copy link
Author

tksharpless commented Oct 9, 2019 via email

@aparrapo
Copy link
Contributor

aparrapo commented Oct 9, 2019

Did you look at the ISPC build instructions for Windows in https://github.com/facebook/facebook360_dep/tree/master/source/thirdparty/bc7_compressor/ISPCTextureCompressor ?

@tksharpless
Copy link
Author

Yeah, that is basically build-it-by-hand. I think I can add a cmake based build but not with a simple modification of the existing ispc.cmake. It seems that Cmake is generating Windows commandlines with '/' where they ought to have '', so they don't execute. Trying to fix this at the level of text munging is an exercise in futility; instead I'll just incorporate the ISPC vcxprojects into the build.
Currently everything is building except ispc and test. I've committed all the changes and will create a pull request.

@tksharpless
Copy link
Author

tksharpless commented Oct 10, 2019 via email

@aparrapo
Copy link
Contributor

Try just "tksharpless"

@tksharpless
Copy link
Author

tksharpless commented Oct 10, 2019 via email

@aparrapo
Copy link
Contributor

I have imported the pull request internally so we can run more automated builds and tests. The results will appear as comments on the pull request itself:
#12

Based on the build failures you can update the code and re-test until everything passes. Then we'll be able to merge it to the main branch.

Anything that has to do with the code itself can be discussed over there instead of here, in case people come back later on and want more context. This issue page can be used to discuss other concepts if needed.

@tksharpless
Copy link
Author

tksharpless commented Oct 10, 2019 via email

@aparrapo
Copy link
Contributor

You can go to the "checks" tab:
https://github.com/facebook/facebook360_dep/pull/12/checks

We use Travis-CI to surface the results of our builds to external users. You'll see the entire build logs with the errors.

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

No branches or pull requests

2 participants