Pcl: vcpkg vtkMapper::ImmediateModeRenderingOff declared deprecated

[KirbX]

Hi all!
I just installed PCL using vcpkg (windows 10, VS2017, x64-static). (And PCL version seems to be 1.8.1-11)
Everything went fine but when I tried to compile the "Using a matrix to transform a point cloud" Example, I get an error as mentionned in the Title :
"vtkMapper::ImmediateModeRenderingOff declared deprecated" from file pcl/visualization/pcl_vizualizer.h.

I only did a copy paste of the project and had this error (in fact, just by including the file pcl_visualizer.h).
Is there a way to correct this compilation error? Maybe replace ImmediateModeRenderingOff by something else?

[taketwo]

@UnaNancyOwen @jasjuang does vcpkg provide 1.8.1 version, or current master?

The problem should have been solved with #2112 and will be a part of 1.9.0 release soon.

[KirbX]

Using .vcpkg list I found that pcl:x64-windows and pcl:x64-windows-static are version 1.8.1-11 as I thought.

Ok, if the problem is solved. But is there a way to make it works since then? I just tried to comment the 2 lines in which the function appeared, but it seems to be a little wobbly :/

[jasjuang]

@KirbX You can install the current master in vcpkg by vcpkg install pcl --head and see if the problem is still there.

[taketwo]

@jasjuang just to clarify, without --head the latest released version is installed, i.e. 1.8.1, right?

[jasjuang]

@taketwo Yes without --head it's 1.8.1.

[KirbX]

I had an error while installing pcl : Here's the screenshot about the error.
The command I used is :
.vcpkg install pcl:x64-windows-static --head

I tried to download it again without the "--head" command and it worked like a charm just as yesterday (installation worked, of course, I still have the error about that vtk function)

[jasjuang]

@KirbX I think @UnaNancyOwen would be the best person to help you here, but I will still try my best to help. Can you post the relevant error message in the log file?

[KirbX]

I don't know what do you mean by "revelant error message", since english is not my native language, but i'll try!

"CMake Error at C:/Program Files/CMake/share/cmake-3.12/Modules/FindPackageHandleStandardArgs.cmake:137 (message):
Could NOT find FLANN (missing: FLANN_LIBRARY) (Required is at least version
"1.7.0")
Call Stack (most recent call first):
C:/Program Files/CMake/share/cmake-3.12/Modules/FindPackageHandleStandardArgs.cmake:378 (_FPHSA_FAILURE_MESSAGE)
cmake/Modules/FindFLANN.cmake:54 (find_package_handle_standard_args)
E:/ProjetGIT/vcpkg/scripts/buildsystems/vcpkg.cmake:247 (_find_package)
CMakeLists.txt:304 (find_package)"

I checked for flann and it was installed, the version 1.9.1.

[jasjuang]

There is a recent PR about flann https://github.com/PointCloudLibrary/pcl/pull/2563 that got merged. Does this help? I think @claudiofantacci can also help you on the Windows side.

[claudiofantacci]

@jasjuang @KirbX not sure exactly what is going wrong there. My fear is that pcl:x64-windows-static --head is not able to get FLANN _static_ and, now that I recall it, I had similar problem in the past and since them I'm using dynamic linking. I'll try to investigate this.

[claudiofantacci]

PCL cannot find static FLANN under vcpkg becuase of these lines:
https://github.com/PointCloudLibrary/pcl/blob/50ca79cfc89a2e850ecdafb613b7a7bab6d8e7ca/cmake/Modules/FindFLANN.cmake#L12-L14
vcpkg installs static libraries in a different installation folder, instead of (say) installed/x64-windows in installed/x64-windows-static. From this folder the name of the libraries is the same of dynamic one, i.e. without the trailing _s.
As a result the PCL FindFLANN needs to be adapted.

[UnaNancyOwen]

@jasjuang Thanks. I have no time for this issue because I'm busy for a while. I really appreciate your help.
@claudiofantacci Thanks. I think it will be fixed by this patch.

[claudiofantacci]

@claudiofantacci Thanks. I think it will be fixed by this patch.

Yes, but FindFLANN changed and the patch is not applied 🙃, see

@KirbX you need to update the patch file yourself, I guess.

Anyway, because of this we have the following tradeoff:

1. you cannot compile PCL as a static triplet by yourself using vcpkg unless you change FindFLANN yourself. I'm trying to think for a possible solution, but it may not be that trivial.
2. you can compule PCL static through vcpkg command with an updated patch, but if you want to personalize some settings you have to change the portfile yourself.

IMHO it would be nice to address 1.

[UnaNancyOwen]

I confirmed this problem.

@claudiofantacci find_flann.patch#L16-L24 has already been included in the upstream head. Therefor, This patch doesn't apply when vcpkg install pcl with --head.

I propose an idea to separate this patch into the first half (remove _s from library name) and the second half (add search directory debug/lib). So, The first half part of patch that necessary for vcpkg will be applied when all case.

• find_flann_remove_library_name_suffix.patch

-  set(FLANN_RELEASE_NAME flann_cpp_s)
-  set(FLANN_DEBUG_NAME flann_cpp_s-gd)
+  set(FLANN_RELEASE_NAME flann_cpp)
+  set(FLANN_DEBUG_NAME flann_cpp-gd)

• find_flann_add_search_path_suffix.patch

-        PATH_SUFFIXES lib)
+        PATH_SUFFIXES lib debug/lib)

What do you think?

[taketwo]

Any reason not to merge the second one directly into PCL?

[UnaNancyOwen]

@taketwo The second one has already merged in master/head (FindFLANN.cmake#L43). However, It is necessary patch in PCL 1.8.1.

[taketwo]

Oh, I see, sorry. And _vcpkg_ does not support different patch lists for the release and --head builds?

I hope we can release 1.9.0 very soon, at least I see no obstacles to doing so (@SergioRAgostinho do I miss anything?). Then _vcpkg_ port can be immediately updated and this problem won't be relevant anymore.

[SergioRAgostinho]

I was still trying to fix some last minute failures with Windows unit tests alongside @claudiofantacci . I was originally hoping they were quick fixes, but some of them are getting somewhat involved.

I'm ok in abandoning that idea and simply release what we currently have.

[UnaNancyOwen]

@KirbX @claudiofantacci I confirmed that it can be installed with following command by apply this pull request (https://github.com/Microsoft/vcpkg/pull/4528). Please try again.

[KirbX]

Hi! I'll be out of office pour something like 1 week. I'll try again when I'll come back! Thanks for your efforts! It is really pleasant to see that you works that hard on problems!! :) Thanks a lot!

[claudiofantacci]

@claudiofantacci find_flann.patch#L16-L24 has already been included in the upstream head. Therefor, This patch doesn't apply when vcpkg install pcl with --head.

@UnaNancyOwen yes, but the first part of the patch should still be applied and it is not because the file on head changed and the second part was already integrated.

Oh, I see, sorry. And _vcpkg_ does not support different patch lists for the release and --head builds?

@taketwo unfortunately you can't.

However I feel we can integrate a correction here in PCL by doing something like this: when an user uses vcpkg, a variable called VCPKG_TRIPLET (or similar, I don't recall it now) is set to a triplet like x64-windows or x64-windows-static. We could check whether VCPKG_TRIPLET exists (i.e. we are using vcpkg) and if contains the string static (i.e. we are link static) then we can change the name to look for the static library with a different name.
What do you think? Is it just too custom?

I was still trying to fix some last minute failures with Windows unit tests alongside @claudiofantacci . I was originally hoping they were quick fixes, but some of them are getting somewhat involved.

I'm ok in abandoning that idea and simply release what we currently have.

@SergioRAgostinho I agree as well, I can make some PR for the fixes I have at this time and you then choose which one you prefer to merge or postpone. Not a problem for me 😄
Apologize for taking so long, but I'm also quite busy 😅

[ras0219-msft]

And vcpkg does not support different patch lists for the release and --head builds?

This could be accomplished via something like

if(VCPKG_USE_HEAD_VERSION)
   vcpkg_apply_patches(...)
endif()

However, I'd be _really_ suspicious of such a thing. head is really intended as a way for authors+users to conspire and temporarily sidestep the normal release process; since the author is participating in that sidestep, there's no reason to apply patches (the author can fix anything upstream easily by just dropping a commit onto master ;)).

We could check whether VCPKG_TRIPLET exists (i.e. we are using vcpkg) and if contains the string static (i.e. we are link static) then we can change the name to look for the static library with a different name.

The variable is VCPKG_TARGET_TRIPLET, however I'd strongly recommend against this (see below).

Generally, in vcpkg, we _erase_ all link-time decisions that libraries need to make. This ensures that libraries are built in the configuration desired by the end user and don't end up with nasties like ODR violations. One of the common transforms to enable this is to rename libraries such that they always have a single name, completely removing the entire need for an if(FLANN_USE_STATIC) branch from your build.

Therefore, I'd recommend letting us _always_ specify something like -DFLANN_USE_STATIC=OFF and you'll end up always searching for the "shared" names (which we'll conspire to make always select the right ones).

Alternatively, you could take the setting FLANN_USE_STATIC as a preference and search for the shared form as a backup:

find_library(FLANN_LIBRARY_DEBUG
             NAMES flann_cpp_s-gd flann_cpp-gd flann_cpp_s flann_cpp

[taketwo]

@ras0219-msft thanks for the information. I don't think I understand your recommendation against checking the VCPKG_TARGET_TRIPLET variable. Isn't it cleaner to explicitly check if it's set and, if so, set expected library file names to the "shared" names? _vcpkg_ is an "exception" in that it performs the name transform, so I think it's reasonable to have a special branch in CMake scripts to handle it.

BTW this reminded me of our long-standing plans to update the way linking requirements are specified/handled in CMake: https://github.com/PointCloudLibrary/pcl/issues/2089#issuecomment-345431295.

[ras0219-msft]

We also try to _not_ be special (though the irony is not lost on me ;p).

The idea here is that while we're special in that static flann has the same name as shared flann, _presumably_ shared flann is something you wanted to support in general (probably the result of that other issue you linked!). In this way, we hope to not be "special" and instead mesh seamlessly into functionality that you wanted to have anyway. Sorry if I'm not being clear about it; more concretely the objective is that you don't see vcpkg support as a burden. We want to minimize the upstream trouble of supporting vcpkg by meshing well into some reasonable configuration on your side that you would want to support anyway. In this example, it would be an option like -DFLANN_USE_STATIC=OFF where users could use shared flann even when building static pcl (a rare but not unreasonable configuration).

However, if you find it easier/preferable to treat vcpkg specially then yes, using VCPKG_TARGET_TRIPLET is the way to go!

[stale[bot]]

Marking this as stale due to 30 days of inactivity. It will be closed in 7 days if no further activity occurs.

[kunaltyagi]

@PointCloudLibrary/testers-windows Could you try to go through this thread to see if this is still actionable?

We have a FLANN_USE_STATIC https://github.com/PointCloudLibrary/pcl/blob/master/CMakeLists.txt#L301 but I'm not knowledgeable in both CMake and Windows

[larshg]

I think the most correct action would be to rejoin the battle in this one https://github.com/microsoft/vcpkg/issues/9810.

And create PR that enables VCPKG to build flann both as static and dynamic, with originally named files.

If not venturing into the VCPKG battle, then I guess we could search for flann_s and if it doesn't find it, print a warning that it wasn't found and that it will now try to search for flann and hope that it picks up the static version (if vcpkg is used).

I normally go for dynamic, but since looking into this, it seems that VCPKG only build static version and PCL doesn't complain for atm.

[kunaltyagi]

VCPKG only build static version and PCL doesn't complain for atm

Sorry for missing the obvious point, but since a lot of people are using PCL via vcpkg, what enhancement do the 2 options (either a bodge in PCL or upstream fix) provide over current status quo?

[larshg]

It seems that this was for a previous version of PCL. VCPKG currently has PCL 1.9.1 and its removing the cmake FindFLANN file all together: https://github.com/microsoft/vcpkg/blob/69e86fd2013e0c495cc4e61500e794548b62ae03/ports/pcl/portfile.cmake#L16.

In addition it adds a patch for FLANN https://github.com/microsoft/vcpkg/blob/69e86fd2013e0c495cc4e61500e794548b62ae03/ports/pcl/use_flann_targets.patch.

So I think this one can just be closed.

Edit: also the OP was not about flann, but a fix to VTK which was already underway :)