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 condition to check dependency MeshSDFilter folder #1704

Merged
merged 5 commits into from
Apr 28, 2024

Conversation

JackBoosY
Copy link
Contributor

Implementation remarks

FTR #767 (comment)

The check should requires ALICEVISION_USE_MESHSDFILTER is ON.

@servantftechnicolor
Copy link
Contributor

Sorry, but this PR is not valid or at least incomplete.

I think this condition is meant to check that the submodules have been retrieved and exit early. There is not only MeshSDFilter but also NanoFlann. If we merge this PR, then we should add a second test for nanoflann submodule.

@@ -178,7 +178,7 @@ endmacro(add_target_properties)
# ==============================================================================
# Check that submodule have been initialized and updated
# ==============================================================================
if(NOT EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/dependencies/MeshSDFilter/CMakeLists.txt)
if(ALICEVISION_USE_MESHSDFILTER AND NOT EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/dependencies/MeshSDFilter/CMakeLists.txt)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, but this PR is not valid or at least incomplete.

I think this condition is meant to check that the submodule_s_ have been retrieved and exit early. There is not only MeshSDFilter but also NanoFlann. If we merge this PR, then we should add a second test for nanoflann submodule.

But this is only for MeshSDFilter, right?

@simogasp
Copy link
Member

Sorry, but this PR is not valid or at least incomplete.

I think this condition is meant to check that the submodule_s_ have been retrieved and exit early. There is not only MeshSDFilter but also NanoFlann. If we merge this PR, then we should add a second test for nanoflann submodule.

That's the point. Nanoflann is mandatory and we don't check it, MeshSDFilter and we check. So either we don't check either of them or we check both of them. MeshSDFilter should be checked the way this PR suggests because it's optional, if anything we should add a check for nanoflann

@JackBoosY
Copy link
Contributor Author

So either we don't check either of them or we check both of them.

Should I also add check for NanoFlann in this PR?

@JackBoosY
Copy link
Contributor Author

Ping @simogasp for review again.

@simogasp simogasp added this to the 3.3.0 milestone Apr 28, 2024
@simogasp simogasp merged commit 70f8b90 into alicevision:develop Apr 28, 2024
4 checks passed
@simogasp
Copy link
Member

@JackBoosY Thanks!

@JackBoosY JackBoosY deleted the jack/fix_condition branch April 29, 2024 02:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants