cmakelists.txt missing find_package(zlib REQUIRED) #10
Comments
|
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. |
|
Two more lines in cmakelists.txt contain gcc-isms that give msvcc commandline errors: However this is no problem compared to the fact that Folly is rife with code that will not compile on vc 14. Suggestions welcome. |
|
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. |
|
Hi Albert,
Although I was able to get legal VC projects by adding one line to
cmakelists.txt and removing two others, it appears to me that it will be
impossible to build them due to a large number of source
incompatibilities. Many of these I believe come from the use of
idioms only understood by gcc. The Folly headers in particular seems to be
full of those.
Surprisingly, vcpkg was able to build the Folly libraries from source
without any reported errors, using the same vc 14 compiler. So it may be
that the vcpkg port includes patches that fix these incompatibilities -- or
perhaps Folly does not call any of the problematic code internally. This
might be worth looking into.
Anyhow my immediate purpose is not to build facebook360-dep but just to
extract from it the presumably good algorithms -- and tested code -- for
keypoint based calibration. My own efforts toward a 6-dof pipeline
definitely need better camera alignment than I can get using only PTGui
control points. I am targeting still panoramas taken with rigs that are
quite "loose" compared to the Red Manifold, so I need to do a bundle
adjustment for every set of photos. I have had no trouble simplifying the
keypoint finder code to remove dependences on Folly, your utilities
libraries and Eigen data types (I prefer to stick with pure OpenCV). I
hope my luck holds for the keypoint matcher.
Very glad you guys have made this code base available.
best,
-- Tom
…On Mon, Sep 30, 2019 at 10:52 AM Albert Parra Pozo ***@***.***> wrote:
Hi @tksharpless <https://github.com/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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZTSKYVOWSQCFWYE2FLQMIHCJA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD755RSA#issuecomment-536598728>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZWUB6E5OTSCHZG5ZZTQMIHCJANCNFSM4I3UGFIA>
.
|
|
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. so you can probably replace the calls with standard If you have any issues removing dependencies let us know and we'll try to make it work :) |
|
Hi Albert,
The issue that concerns me most is generating calibrations for the kinds of
rigs we now use to capture stereo still panoramas. A full "bubble camera"
array is necessary for video, but you can shoot stereo stills many ways by
sweeping partial arrays over the sphere. I have used rigs with 1, 2, 3 and
4 physical cameras to capture the kind of data that we agree is needed for
6-dof, that is, sets of heavily overlapped fisheye photos. Others have
experimented with sweeping narrow angle circular arrays of the type now
used for 360 video, to get more overlap. I am not much interested in that
approach, but I really do want to serve the most common types of fisheye
rigs: a single camera offset from rotation center in its view direction,
and a stereo pair of cameras offset transverse to their view direction.
Both types need a pitch pivot in order to take multiple rows as required
for 6-dof; however for practical reasons that pivot is often offset from
the rotation axis. The s/w I have been developing uses a rig definition
that includes possible offset pitch and roll pivots, and computes camera
positions by rotating a model around 3 pivots by the estimated view
rotations. It would not be hard to add such specifications to your rig
model -- and since it is json, it should be possible to do that without
breaking existing code (assuming that Folly parses json honestly). So I'm
working on an extended rig object spec.
I shall also be proposing an extended Camera object, that has a universal
lens model. It is of paramount practical importance to make it possible
for the owner of a camera and lens to calibrate them. I have been
wrestling with this issue since 2008 and now have pretty firm opinions
about how it needs to be handled. Having tried several technically correct
fisheye calibration schemes I can state categorically that less than one
pro photographer in 100 would willingly do that -- even if his stitching
s/w "requires" it. My own software relies on a method that panoramic
photographers can understand and accept: using PTGui to stitch a normal 360
series taken "on nodal" with plenty of overlap, with optimization of all
lens parameters enabled. I no longer let it bother me that PT style lens
calibrations are slightly suspect on scientific grounds, and not fully
portable or scalable. They are accessible, and they work. So my Camera
object takes a PTGui calibration and converts its correction polynomial to
a pair of cubic splines (radius=>angle and angle=>radius). The basic lens
curve is given by a single-parameter function due to Donald Gennery (JPL)
that can model any real lens curve pretty closely. I like to say that my
camera object has a universal lens model, that could be set up from many
varieties of calibration data -- including the present fb360-dep model, of
course. I think this is an ideal export model because it does not
presuppose any particular way of calibrating the lenses, but should be
adaptable to any way, even the most precise (laser interferometry, anyone?).
…On Tue, Oct 1, 2019 at 5:15 PM Albert Parra Pozo ***@***.***> wrote:
Hi @tksharpless <https://github.com/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 :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZWN4RBR6TWW5RH5AHTQMO4W7A5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEACYTQQ#issuecomment-537233858>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZVQR5OCUPORKCJMDXDQMO4W7ANCNFSM4I3UGFIA>
.
|
|
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 :) |
|
Hi Albert
On second thoughts, and more careful reading of the code, I can see that
there is nothing about your camera model that needs fixing. Good job.
I would not trust lens calibrations based on empirical alignment of photos
taken from different viewpoints, in PTGui or any other s/w. There is no
way to correct control points for parallax with sufficient accuracy. Your
way of calibrating the Manifold is OK because you are starting from a very
accurate geometrical model of the rig, but with less accurate nominal
models I don't think this approach would give reliable lens parameters.
The same applies to the more basic problem of 'rectifying' the photos,
which is to say, aligning them at infinity. Even with good lens
correction, I have found it hard to convince myself that bundle adjustment
using PTGui control points in an epipolar error model produces any
verifiable improvement over PTGui's alignments -- although it will correct
gross errors in my initial rig model. (When the camera view direction is
radial. When it is not, as in two camera stereo, the epipolar alignment is
clearly better than PTGui's, but still not good enough).
I am hoping to do better by using control points found the fb360-dep way,
which should be better than PTGui's for a number of reasons. However I
really believe that it will never be possible to build SFM-type 3D surface
models of truly photorealistic quality from bubble-camera data, and that we
need to be looking for ways to render realistic images from lower quality
models.
…On Wed, Oct 2, 2019 at 12:05 PM Albert Parra Pozo ***@***.***> wrote:
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 :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZUWDUHPYFQHNZC5AI3QMTBDTA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAFJGBY#issuecomment-537563911>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZQSX4MNDATAE3T4NHDQMTBDTANCNFSM4I3UGFIA>
.
|
|
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. |
|
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 |
|
Yeah, Ifound that
But this will not work because the code has "boost::filesystem::" in many places. BUT actually must use boost anyhow. 4 of your headers explicitily include <boost/filesystem.hpp> So bottom line is use boost::filesystem and provide that string conversion function on Windows. |
|
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. |
|
BTW the string conversion problem comes from the fact that boost::filesystem::path is wchar on Windows but char on nixoids. |
|
I have been able to compile the calibration library for Windows with VC
2017 with reasonably few code changes. The most significant is eliminating
the multithreaded driver function. I plan to use this library to build
lightweight file-to-file commands that process one or two images at a time,
to be run in parallel by an xargs clone, jom or the like, plus a driver
program that sets up and collates their work.
I attach a git patch for my changes including the ones to cmakelists.txt.
I believe that it is compatible with your standard Linux and Mac builds but
have not tested that.
You probably don't want to add me as an authorized developer since my goals
diverge from yours quite a bit, and I really don't have the time for proper
regression testing (I'm 80 and want to get this done before I lose it).
…-- Tom
On Wed, Oct 2, 2019 at 9:15 PM Albert Parra Pozo ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZW5UPFWUBTZV7AH7QTQMVBTXA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAGVM4Q#issuecomment-537745010>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZVULKPP5EAZR6UZTE3QMVBTXANCNFSM4I3UGFIA>
.
|
|
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 :) |
|
It appears that I will be able to build your multithreaded calibration
program as-designed. The issue with future that was worrying me is trivial
to get around -- just define an empty default c'tor for Overlap (I suspect
g++ silently does that, vc++ does not). Since the contents of Overlap are
self-initializing, this works fine.
So I retract the parts of the patch I sent you that apply to
FeatureMatcher.cpp. The original file is fine.
Your Camera.h suppresses saveRig() when WIN32 is defined, but this gives a
link error so
I have changed that.
The main issue I have encountered today is a bug in cmake that puts an
extra reference to double-conversion.lib on the link command lines, without
the required path to the vcpkg install directory. Have not found a way to
make cmake stop doing that, but it is simple to correct in Visual Studio.
The only remaining link errors for Calibration.exe are several unresolved
FLAGS_ references, which I'm now tracking down. When those are fixed I
will send you a new comprehensive patch.
…On Thu, Oct 3, 2019 at 9:18 PM Albert Parra Pozo ***@***.***> wrote:
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 :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZURZB4SMGZJ2RHTS6DQM2KXLA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAKCASI#issuecomment-538189897>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZQL37BA4AVTPT4Y6WLQM2KXLANCNFSM4I3UGFIA>
.
|
|
The unresolved references are just the nine flags DECLARE_ed in
Calibration.h and DEFINE_ed in Calibration.cpp. And all those definitions
are flagged with a warning about inconsistent DLL linkage.
It now comes back to me that it is insanity to try to use gflags as a dll;
however the vcpkg scripts build the dll and gflags headers default to
assuming dll linkage, putting you into a subcircle of dll hell. This even
if you link agains the static library. It gets worse. You are supposed to
be able to force static linkage by setting preprocessor
symbol GFLAGS_IS_A_DLL to 0, so I put that in the CXXFLAGS in the fb360-dep
cmakelists.txt. But in the generated vcxprojs, /DGFLAGS_IS_A_DLL=0 is
followed immdiately by
/DGFLAGS_IS_A_DLL=1. Not sure why they are so anxious to keep the madness
going. If I delete that the calibration builds ok but for one final
undefined reference: FLAGS_v.
Ain't THAT nasty. It is defined by glog, which is linked as a dll, so
presumably uses gflags.dll, which is no longer visible to the referencing
program. The right solution of course is to link glog statically as well.
However since this reference is just used to prettify some ceres solver
printout, I am going to punt that for now and simply define a dummy local
FLAGS_v in that module.
OK that does it. I have an exe. Let you know tomorrow if it runs.
On Fri, Oct 4, 2019 at 2:21 PM Thomas Sharpless <[email protected]>
wrote:
… It appears that I will be able to build your multithreaded calibration
program as-designed. The issue with future that was worrying me is trivial
to get around -- just define an empty default c'tor for Overlap (I suspect
g++ silently does that, vc++ does not). Since the contents of Overlap are
self-initializing, this works fine.
So I retract the parts of the patch I sent you that apply to
FeatureMatcher.cpp. The original file is fine.
Your Camera.h suppresses saveRig() when WIN32 is defined, but this gives a
link error so
I have changed that.
The main issue I have encountered today is a bug in cmake that puts an
extra reference to double-conversion.lib on the link command lines, without
the required path to the vcpkg install directory. Have not found a way to
make cmake stop doing that, but it is simple to correct in Visual Studio.
The only remaining link errors for Calibration.exe are several unresolved
FLAGS_ references, which I'm now tracking down. When those are fixed I
will send you a new comprehensive patch.
On Thu, Oct 3, 2019 at 9:18 PM Albert Parra Pozo ***@***.***>
wrote:
> 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 :)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#10?email_source=notifications&email_token=ACBEZZURZB4SMGZJ2RHTS6DQM2KXLA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAKCASI#issuecomment-538189897>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACBEZZQL37BA4AVTPT4Y6WLQM2KXLANCNFSM4I3UGFIA>
> .
>
|
|
Well wouldn't you know it. Nice that it starts up without crashing, but
then....
PS C:\Users\tommy\build-FB360-dep\bin\Debug> ./calibration
ERROR: flag 'v' was defined more than once (in files
'C:\Users\tommy\vcpkg\buildtrees\glog\src\v0.4.0-46ccbc49a4\src\vlog_is_on.cc'
and 'C:\FB-6dof\facebook360\source\calibration\GeometricCalibration.cpp').
On Fri, Oct 4, 2019 at 4:51 PM Thomas Sharpless <[email protected]>
wrote:
… The unresolved references are just the nine flags DECLARE_ed in
Calibration.h and DEFINE_ed in Calibration.cpp. And all those definitions
are flagged with a warning about inconsistent DLL linkage.
It now comes back to me that it is insanity to try to use gflags as a dll;
however the vcpkg scripts build the dll and gflags headers default to
assuming dll linkage, putting you into a subcircle of dll hell. This even
if you link agains the static library. It gets worse. You are supposed to
be able to force static linkage by setting preprocessor
symbol GFLAGS_IS_A_DLL to 0, so I put that in the CXXFLAGS in the fb360-dep
cmakelists.txt. But in the generated vcxprojs, /DGFLAGS_IS_A_DLL=0 is
followed immdiately by
/DGFLAGS_IS_A_DLL=1. Not sure why they are so anxious to keep the madness
going. If I delete that the calibration builds ok but for one final
undefined reference: FLAGS_v.
Ain't THAT nasty. It is defined by glog, which is linked as a dll, so
presumably uses gflags.dll, which is no longer visible to the referencing
program. The right solution of course is to link glog statically as well.
However since this reference is just used to prettify some ceres solver
printout, I am going to punt that for now and simply define a dummy local
FLAGS_v in that module.
OK that does it. I have an exe. Let you know tomorrow if it runs.
On Fri, Oct 4, 2019 at 2:21 PM Thomas Sharpless ***@***.***>
wrote:
> It appears that I will be able to build your multithreaded calibration
> program as-designed. The issue with future that was worrying me is trivial
> to get around -- just define an empty default c'tor for Overlap (I suspect
> g++ silently does that, vc++ does not). Since the contents of Overlap are
> self-initializing, this works fine.
>
> So I retract the parts of the patch I sent you that apply to
> FeatureMatcher.cpp. The original file is fine.
>
> Your Camera.h suppresses saveRig() when WIN32 is defined, but this gives
> a link error so
> I have changed that.
>
> The main issue I have encountered today is a bug in cmake that puts an
> extra reference to double-conversion.lib on the link command lines, without
> the required path to the vcpkg install directory. Have not found a way to
> make cmake stop doing that, but it is simple to correct in Visual Studio.
>
> The only remaining link errors for Calibration.exe are several unresolved
> FLAGS_ references, which I'm now tracking down. When those are fixed I
> will send you a new comprehensive patch.
>
> On Thu, Oct 3, 2019 at 9:18 PM Albert Parra Pozo <
> ***@***.***> wrote:
>
>> 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 :)
>>
>> —
>> You are receiving this because you were mentioned.
>> Reply to this email directly, view it on GitHub
>> <#10?email_source=notifications&email_token=ACBEZZURZB4SMGZJ2RHTS6DQM2KXLA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAKCASI#issuecomment-538189897>,
>> or mute the thread
>> <https://github.com/notifications/unsubscribe-auth/ACBEZZQL37BA4AVTPT4Y6WLQM2KXLANCNFSM4I3UGFIA>
>> .
>>
>
|
|
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. |
|
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 :) |
|
Finally have a working calibrate.exe -- testing it now on your sample data.
The last piece of the gflags puzzle is solved by putting the definitions
from calibrate.cpp into one of the other modules -- apparently the define
statements by themselves do not make msvcc create a loadable module. This
works with the gflags dll built by vcpkg, no need to change to static
linking (whew...).
I want to review my changes to make sure they look compatible with the
Linux build and don't include anything unnecessary, then I will create a
pull request.
…On Sun, Oct 6, 2019 at 8:12 PM Albert Parra Pozo ***@***.***> wrote:
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 :)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZUCDADPJWHB52X5XDDQNJ5F3A5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAOXV4Q#issuecomment-538802930>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZU7J2SJ5A5UWWZCTS3QNJ5F3ANCNFSM4I3UGFIA>
.
|
|
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. |
|
Rebuilt ceres with SuiteSparse. Now geometrical calibration demands less that 1GB of RAM.
I await your advice. |
|
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 can now build all of facebook360-dep except test. The only real source
code issues are dealing with the wchar strings that boost-win32 uses for
path names, which is simple, and and getting all the flags properly linked,
which is anything but.
The solution that the gflags maintainers recommend, having one module
DEFINE_ a set of flags that others DECLARE_, works but is a very bad one
for library code, as there is no way to predict which modules will be
linked in a given build. So I am looking for a better one that will
support building all of fb360-dep and other apps that might be derived from
it, without resorting to ad-hoc tweaks.
The root problem appears to be that in the WIN32 context DEFINE_xxxx() does
not generate loadable code, therefore the linker ignores modules consisting
only of such statements. The only way to get them to take seems to be to
put them inside something that will really be executed.
On Sun, Oct 6, 2019 at 9:05 PM Thomas Sharpless <[email protected]>
wrote:
… Finally have a working calibrate.exe -- testing it now on your sample data.
The last piece of the gflags puzzle is solved by putting the definitions
from calibrate.cpp into one of the other modules -- apparently the define
statements by themselves do not make msvcc create a loadable module. This
works with the gflags dll built by vcpkg, no need to change to static
linking (whew...).
I want to review my changes to make sure they look compatible with the
Linux build and don't include anything unnecessary, then I will create a
pull request.
On Sun, Oct 6, 2019 at 8:12 PM Albert Parra Pozo ***@***.***>
wrote:
> 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 :)
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#10?email_source=notifications&email_token=ACBEZZUCDADPJWHB52X5XDDQNJ5F3A5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAOXV4Q#issuecomment-538802930>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACBEZZU7J2SJ5A5UWWZCTS3QNJ5F3ANCNFSM4I3UGFIA>
> .
>
|
|
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. |
|
The correct fix for wide boost::filesystem strings is the official boost
solution, which is to imbue filesystem with the Windows UTF-8 codepage.
This will require that all code which needs a std::string must call the
string() function on the filesystem item. This code should work under
Linux.
Another new wrinkle is the need to build and install the Windows version of
the Intel ISPC libraries. ISPC is not available in vcpkg, so it has to be
done by hand, in several steps.
The only viable solution for the gflags linking problem appears to be
ad-hoc adding the needed DEFINE_s to each main program that calls anything
that needs them. This will result in a lot of file changes, all
conditional on #ifdef WIN32 however so should not break Linux builds. If
you ever get to refactor facebook360-dep I would strongly urge adopting a
more systematic and global approach to defining gflags.
…On Tue, Oct 8, 2019 at 8:44 PM Albert Parra Pozo ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZUXR7ZQMY4N5J5J5LTQNUSPTA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAWCVPI#issuecomment-539765437>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACBEZZSGL6LESVVVPHN566DQNUSPTANCNFSM4I3UGFIA>
.
|
|
The situation with ISPC is a bit tricky. Your ISPC.cmake script creates
projects for building the ispc libraries on Linux and OS X; but the process
is different on Windows. Intel does not supply a Cmake script for it,
only VC projects, so building it involves invoking msbuild on 4 vcxproj
files.
The easy way out is to build the ISPC projects manually under Visual Studio
then copy the resulting generated headers, dll and import lib to the right
places in the fb360-dep build tree. That is what I will do initially, but
I will put a copy of the relevant parts of the ISPC distro in the fb360-dep
git repo as a basis for automating this in the future. I will also include
a VS solution for manual building.
On Wed, Oct 9, 2019 at 10:36 AM Thomas Sharpless <[email protected]>
wrote:
… The correct fix for wide boost::filesystem strings is the official boost
solution, which is to imbue filesystem with the Windows UTF-8 codepage.
This will require that all code which needs a std::string must call the
string() function on the filesystem item. This code should work under
Linux.
Another new wrinkle is the need to build and install the Windows version
of the Intel ISPC libraries. ISPC is not available in vcpkg, so it has to
be done by hand, in several steps.
The only viable solution for the gflags linking problem appears to be
ad-hoc adding the needed DEFINE_s to each main program that calls anything
that needs them. This will result in a lot of file changes, all
conditional on #ifdef WIN32 however so should not break Linux builds. If
you ever get to refactor facebook360-dep I would strongly urge adopting a
more systematic and global approach to defining gflags.
On Tue, Oct 8, 2019 at 8:44 PM Albert Parra Pozo ***@***.***>
wrote:
> 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.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#10?email_source=notifications&email_token=ACBEZZUXR7ZQMY4N5J5J5LTQNUSPTA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAWCVPI#issuecomment-539765437>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/ACBEZZSGL6LESVVVPHN566DQNUSPTANCNFSM4I3UGFIA>
> .
>
|
|
Did you look at the ISPC build instructions for Windows in https://github.com/facebook/facebook360_dep/tree/master/source/thirdparty/bc7_compressor/ISPCTextureCompressor ? |
|
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. |
|
See my latest post.
Can't create a pull request because the FB CLA submission page is rejecting
my submission, claiming that
"[email protected]" is not a valid github username. False. That is
my github username.
I don't know if you can do anything about it, but I'm not going to change
my github account just to satsify
this ignorant form.
…On Wed, Oct 9, 2019 at 6:18 PM Albert Parra Pozo ***@***.***> wrote:
Did you look at the ISPC build instructions for Windows in
https://github.com/facebook/facebook360_dep/tree/master/source/thirdparty/bc7_compressor/ISPCTextureCompressor
?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZWIR5RDQEVHQOTY4FLQNZKB3A5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAZTXSQ#issuecomment-540228554>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBEZZVRNVODWOSQSTUPWVTQNZKB3ANCNFSM4I3UGFIA>
.
|
|
Try just "tksharpless" |
|
OK Albert,
I have created a fork and branch, pushed my changes to it and opened a
pull request.
Which has already been found not to build on Linux. No surprise.
What is the procedure for discussing/resolving issues?
Today I am working on automating the installation of required packages by
vcpkg thru an addtional cmake script. This should be a big help for the
average user. As you know, vcpkg builds a directory hierarchy called an
instance that contains itself and a set of installed packages. Instances
are independent and self-sufficent, this is an ideal way to ensure that the
right packages are used for a given build. The complete Windows build
package will then consist of 3 directories
-- facebook360-dep clone
-- vcpkg instance
-- build directory
The latter two generated by cmake scripts in the former. I would put all
3 under one umbrella directory by default. Of course it will be possible
to designate an existing vcpkg instance. BTW vcpkg also works on Linux and
OS X, so you might want to consider using it for all builds.
I'm going to try to get cmake to put the official Windows build projects
for ISPC into the fb360-dep solution, so that it all builds automatically
on Windows as on Linux. Failing that some manual step will be needed.
Finally, must make the gtest-based tests work on Windows.
…On Wed, Oct 9, 2019 at 9:49 PM Albert Parra Pozo ***@***.***> wrote:
Try just "tksharpless"
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZRSNYK4RTFBJD7XL53QN2C4FA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA2CR6I#issuecomment-540289273>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBEZZSHBPYGVQMQ7QDVHCDQN2C4FANCNFSM4I3UGFIA>
.
|
|
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: 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. |
|
Thanks, Albert.
It appears that as a non-fb employee I can't view the results of automated
tests -- I probably would not understand them anyhow. So how will I be
informed of failure details?
…On Thu, Oct 10, 2019 at 10:38 AM Albert Parra Pozo ***@***.***> wrote:
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 <#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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#10?email_source=notifications&email_token=ACBEZZXNM27L7MQB6JFDKTLQN446JA5CNFSM4I3UGFIKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEA4S7PY#issuecomment-540618687>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACBEZZV64JGMGIDUDDXXEWTQN446JANCNFSM4I3UGFIA>
.
|
|
You can go to the "checks" tab: We use Travis-CI to surface the results of our builds to external users. You'll see the entire build logs with the errors. |
No description provided.
The text was updated successfully, but these errors were encountered: