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

Add new CMake option to disable GUI #1165

Merged
merged 16 commits into from Mar 12, 2021
Merged

Conversation

anmatako
Copy link
Contributor

@anmatako anmatako commented Mar 8, 2021

The dependencies to Qt and the GUI build are now conditional on the new GUI_ENABLED option.

@anmatako
Copy link
Contributor Author

anmatako commented Mar 9, 2021

@ahojnnes Seems to me that with the current colmap setup, OpenGL and Qt are intertwined so when disabling the GUI and skipping Qt we cannot really use OpenGL as well. Is this understanding accurate?

I can see a case of problematic behavior with my changes when we build without GUI support and without CUDA as well. This is the case where kUseOpenGL becomes true and the underlying Qt classes are stubbed out so things would fail.

What would be the best way to handle that? It seems to only affect feature extraction and matching that can use GPU either from CUDA or OpenGL. My idea would be to simply check the use_gpu option and ensure it's always false when both GUI_ENABLED and CUDA_ENABLED are undefined. This would force Sift to use cpu for extraction and matching which would not cause any kind of failure.

@whuaegeanse
Copy link
Contributor

@anmatako
At least two Qt related codes need to be changed. The first is the code related to SiftGPU. The second place is the code related to IncrementalMapperOptions.

@anmatako
Copy link
Contributor Author

anmatako commented Mar 9, 2021

@anmatako
At least two Qt related codes need to be changed. The first is the code related to SiftGPU. The second place is the code related to IncrementalMapperOptions.

@whuaegeanse The mapper options you linked are forward declarations and when the GUI is disabled the "ui" folder is excluded form the sources so they are never actually used. So, I don't think there's a need to add any special handling for them.

However, I do plan to make some additional changes to better handle the feature matching and extraction when both GUI and CUDA are disabled.

@anmatako
Copy link
Contributor Author

anmatako commented Mar 9, 2021

Latest update now handles the case of both CUDA and OpenGL missing when attempting to use Sift GPU. In this case there is an error and EXIT_FAILURE with a message to disable the use_gpu option.

CMakeLists.txt Outdated Show resolved Hide resolved
src/ui/render_options.h Outdated Show resolved Hide resolved
@whuaegeanse
Copy link
Contributor

@anmatako @ahojnnes Consider using SIFTGPU's original opengl-related code, which has nothing to do with Qt. In this way, we can use opengl to accelerate feature extraction and feature matching without relying on Qt.I have done relevant experiments and the results are very good, and I am happy to submit relevant code for SIFTGPU that does not rely on Qt.

@anmatako
Copy link
Contributor Author

@anmatako @ahojnnes Consider using SIFTGPU's original opengl-related code, which has nothing to do with Qt. In this way, we can use opengl to accelerate feature extraction and feature matching without relying on Qt.I have done relevant experiments and the results are very good, and I am happy to submit relevant code for SIFTGPU that does not rely on Qt.

@whuaegeanse I don't think the problem in this case is the implementation of Sift GPU that colmap is using. If I understand correctly, colmap uses the default implementation you are linking and it does indeed depend on OpenGL and builds with it. The problem as I see it is with colmap's use of OpenGL that only happens through Qt and thus is tied to the GUI. So, when I'm disabling the GUI I am also (as a side-effect) removing the ability to use OpenGL as well.

There should be a way to decouple OpenGL from Qt in colmap, but such a change goes beyond the scope of this PR. My intent here is much simpler and along the lines of:
"I don't want the colmap GUI and I accept that this also means that OpenGL will be unavailable as well".

Of course the behavior of disabling the GUI needs to be properly documented, so I'm open to suggestions as to where would be the best place to add such a note.

@ahojnnes please correct me if my understanding of this issue is incorrect.

@whuaegeanse
Copy link
Contributor

@anmatako You are right, after disabling Qt, feature extraction and feature matching that rely on opengl will not be available.
But disabling Qt should not affect the use of feature extraction and feature matching, because the original version of SIFTGPU has nothing to do with related interface libraries such as Qt. Therefore, we should provide an OpenGLContextManager that does not rely on Qt.

@anmatako
Copy link
Contributor Author

@whuaegeanse I agree with you that it should be possible to implement a context manager that does not rely on Qt, but that should be a separate issue and PR.

@ahojnnes ahojnnes merged commit c354f6f into colmap:dev Mar 12, 2021
@anmatako anmatako deleted the public/no-gui branch March 12, 2021 14:50
lucasthahn pushed a commit to TNE-ai/colmap that referenced this pull request Aug 17, 2022
* Add new CMake option to disable GUI

* Update sift.cc

* Disable Sift GPU when both CUDA and OpenGL are not available

* Update CMakeLists.txt

* Create CMakeLists.txt

* Fix merge issue

* Address PR comments

* revert incorrect change

* Add blank line at end of CMake file

Co-authored-by: Antonios Matakos <anmatako@dss.microsoft.us>
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.

None yet

3 participants