[OpenCV] fix compatibility with VTK9

[cenit]

After VTK9 update in #12149, VTK feature in OpenCV was broken. This patch will fix it.

[JackBoosY]

LGTM.

[cenit]

back to draft: there are some problems in downstream usage. Might require deeper fixes unfortunately

[JackBoosY]

@cenit Please ping me if this PR is ready for review.

[Neumann-A]

@cenit: any application using the viz module which can be put into scripts\test_ports ?

[cenit]

I can look for one. The one I am working on right now is not open source :)

[cenit]

I had to patch vtk port, it was failing otherwise in downstream projects using the library...

[cenit]

A simple test project would have detected that vtk had green mark from CI but was not working in reality, since the linking stage was failing

[cenit]

@JackBoosY PR might be ready. I have enabled VTK also on static builds. Initial tests look fine, both on OpenCV3 and on OpenCV4.

[Neumann-A]

A simple test project would have detected that vtk had green mark from CI but was not working in reality, since the linking stage was failing

WTF? ParaView is not enough for you? It is also building a runable executable... Furthermore #11148 and #9960 were reviewed by one of the VTK maintainers.
So if there are problems with VTK please do the following:

1. make sure it is not vcpkg related
2. make sure it is not a case of wrong usage.
3. search for upstream issues in https://gitlab.kitware.com/groups/vtk/-/issues

• if there is none with a similar problem please open up an upstream issue and mention it in the portfile.
• if there is one then there is probably already a patch. If not mention that you also ran into the problem in the upstream issue and patch it in vcpkg accordingly. Also mention the issue in the portfile.

[cenit]

very easy check: try building this sample code without that fix
Might be that Paraview is silently patching this failure? Or that maybe if you only built a dynamically linked .exe it was failing if tried to run?

#include <vtkSmartPointer.h>
#include <vtkTransform.h>
#include <vtkMath.h>

int main()
{
  vtkSmartPointer<vtkTransform> transform = vtkSmartPointer<vtkTransform>::New();
  return 0;
}

it was broken on linux (at least) without that change :)

ps: I also suggest you to moderate your tones. Also, are you sure you have any power to veto or delay a PR so strongly?

[Neumann-A]

Might be that Paraview is silently patching this failure? Or that maybe if you only built a dynamically linked .exe it was failing if tried

ParaView is not silently patching anything of VTK and is building on osx, linux and windows (x64&static; x86 is not tested due to a cascade failure) without issues (see vcpkg CI logs, basically everything rebuilding qt also rebuilds paraview). If it would have been patched by ParaView it would have made it directly into VTK since both ports are highly connected sharing devs.

ps: I also suggest you to moderate your tones.

The tone was more disbelief than anger if you read it with that tone.

Also, are you sure you have any power to veto or delay a PR so strongly?

I don't have any power I can just give strong suggestions ;) But you are neither disclosing a full repro including the CMakeLists.txt and feature set of the installed VTK nor have you opened up an upstream issue until now.
VTK is very active in development so delaying the PR until the root cause for the failure has been discussed or the failure has been identified as a bug and not a usage error seems reasonable to me.
I am just considering probabilities here. From my point of view a usage error on your side has a higher probability than an error in VTK. As such I want the issue fully discussed before any action is taken and I don't mind being wrong in the end.

[Neumann-A]

CMakeLists.txt

cmake_minimum_required(VERSION 3.17)
project(vtk_test)
find_package(VTK CONFIG REQUIRED)
add_executable(vtk_test main.cpp)
target_link_libraries(vtk_test VTK::CommonCore VTK::CommonTransforms VTK::CommonMath)

main.cpp

#include <vtkSmartPointer.h>
#include <vtkTransform.h>
#include <vtkMath.h>

int main()
{
  vtkSmartPointer<vtkTransform> transform = vtkSmartPointer<vtkTransform>::New();
  return 0;
}

Terminal Output:

neumann@Iluinrandir:~/vtk_test$ make
Scanning dependencies of target vtk_test
[ 50%] Building CXX object CMakeFiles/vtk_test.dir/main.cpp.o
[100%] Linking CXX executable vtk_test
[100%] Built target vtk_test

I don't see any issues..... so somebody has to dig deeper.
(Installed VTK: vtk[core,opengl,paraview,qt,vtkm]:x64-linux)

[JackBoosY]

If this PR-related issue is related to vtk, do we need to call vtk official review this PR?

[cenit]

Sorry I am on vacation from today for few days
What if you replace the find package call with
find_package(VTK 9 NAMES vtk COMPONENTS FiltersExtraction FiltersSources FiltersTexture IOExport IOGeometry IOPLY InteractionStyle RenderingCore RenderingLOD RenderingOpenGL2 NO_MODULE REQUIRED)
and then the target creation with

add_executable(vtk_test vtk_test.cpp)
target_include_directories(vtk_test PRIVATE ${VTK_INCLUDE_DIRS})
target_link_libraries(vtk_test PRIVATE ${VTK_LIBRARIES})

?

This config was failing for me without that fix. This is how vtk is used in the opencv community, and it was working before vtk9

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[cenit]

@strega-nil is pangolin regression due to this PR? I doubt it...
Please let me know if this PR can be considered for merging
I also already saw that there are problems with brand new Xcode and Ceres, which are triggering downstream problems in OpenCV. I am not going to fix them here because they are

1. out of topic for this PR (in any case it's just a

set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED ON)

missing at the end of CeresConfig.cmake)
2) still not triggered by CI (maybe it's not updated to latest Xcode)

[strega-nil]

@BillyONeal is the new person on duty, so let's see what he has to say.

[cenit]

after a rerun pangolin disappeared and opencv4 on macOS returned. I am unable to reproduce it locally on my machine.
Because of the regression, I am almost sure that the PR still cannot be accepted

[cenit]

I am trying some "blind fixes" using directly the azure pipeline here to verify them, due to the fact that locally it works.
I will try some experiments exploiting some discussions above which led to some suggestions

[BillyONeal]

Unfortunately I haven't seen a build where pangolin failed; it isn't one of the ones that have often failed in our CI anyway but there could be some form of races in there.

At least in the current build it looks like it doesn't like generator expressions in a place?

CMake Warning at cmake/OpenCVUtils.cmake:1576 (message):
  Unexpected CMake generator expression:
  $<$<NOT:$<CONFIG:DEBUG>>:/Users/vagrant/Data/installed/x64-osx/lib/libwebp.a>
Call Stack (most recent call first):
  CMakeLists.txt:1085 (ocv_get_all_libs)

I'm not sure where OpenCVUtils.cmake comes from...

[BillyONeal]

I also observe pangolin says Supports: !uwp & !osx so I'm not sure how it could affect an osx run..

[cenit]

I also observe pangolin says Supports: !uwp & !osx so I'm not sure how it could affect an osx run..

https://github.com/microsoft/vcpkg/runs/1515708729

this is the run where pangolin failed. In fact, osx went fine there and windows failed.
I will investigate this generator expression regression in opencv and let you know

[cenit]

wait a minute: why is opencv not tested in ANY pipeline apart osx??
It is marked as a cascade failure and so not tested 😨

This PR is arriving very late! So late that OpenCV is already broken on master?? 😢

[cenit]

@BillyONeal in any case now this PR has a green mark, I hope it can be considered for merging because it contains an opencv test which is verifying important features excluded from the default build which otherwise often regress

[JackBoosY]

@BillyONeal Ping for review and merge this PR.

[BillyONeal]

Thanks for your help!