pcl icon indicating copy to clipboard operation
pcl copied to clipboard

Remove deprecated code before PCL 1.13.0 release

Open theoniko opened this issue 3 years ago • 13 comments

Fixes #5313

theoniko avatar Aug 11 '22 18:08 theoniko

Seems we need to upgrade 18.04 to use VTK 7 first. https://github.com/PointCloudLibrary/pcl/blob/a92da8b793cda3567b89ce86b2f9ed9215e1918d/.ci/azure-pipelines/env.yml#L47

Should be available: https://packages.ubuntu.com/bionic/libvtk7-dev

larshg avatar Aug 12 '22 05:08 larshg

To be honest I don't really understand why OpenGL compatibility should be deprecated/removed. PR https://github.com/PointCloudLibrary/pcl/pull/4065 by @SunBlack introduced the deprecation, claiming that we don't loose any compatibility by dropping support of the old backend, and that Ubuntu 18.04 already has OpenGL2 in its repo. However VTK 6.3 in Ubuntu 18.04 and 20.04 both still use OpenGL. So if we remove support for OpenGL, we effectively remove support for VTK 6.3 in Ubuntu, for which I don't see a reason. When a user installs VTK from a package manager (not self compiled), they don't really have a choice which backend they get. And even if the OpenGL backend is not supported in newer VTK versions, I don't see why we have to do anything on the PCL side: for people using VTK from package managers, they will automatically get OpenGL2 for newer VTK versions, and for people self-compiling VTK, they will get warnings or won't be able to build VTK if they try to use OpenGL with newer VTK versions. So I would propose to un-deprecate the OpenGL backend and continue supporting it. Thoughts @larshg @SunBlack ?

mvieth avatar Aug 12 '22 08:08 mvieth

@theoniko Oh and there are some more deprecated things in filters and registration (see the issue). Would you also remove those, please?

mvieth avatar Aug 12 '22 08:08 mvieth

The idea behind dropping the old backend is to simplify the code by removing #if VTK_RENDERING_BACKEND_OPENGL_VERSION < 2, as I don't think any maintainer here really checks every relevant change regarding VTK to see if it still works with the old backend.

But in general I think: If you absolutely want to upgrade to PCL 1.13.0, you should also be able to switch to e.g. VTK 7 or in case of doubt build VTK yourself, if you insist on historical in combinations with current systems ;-)

SunBlack avatar Aug 12 '22 16:08 SunBlack

Ok, that sounds reasonable. Then we have to update the 18.04 docker image to VTK 7, as Lars suggested, but in a separate pull request. @theoniko would you also be interested to do that? And please remove the deprecated function in bfgs.h

mvieth avatar Aug 13 '22 09:08 mvieth

@mvieth: PR is not yet ready anyway. I can give it a try to update 18.04 docker image to VTK 7 with a new pr.

theoniko avatar Aug 13 '22 10:08 theoniko

@theoniko : Can you also removing all checks for #if VTK_RENDERING_BACKEND_OPENGL_VERSION < 2 as you removed the sipport for this in CMake as well?

SunBlack avatar Aug 15 '22 10:08 SunBlack

@theoniko Are you still working on this?

mvieth avatar Sep 12 '22 15:09 mvieth

@mvieth: I am quite busy these days. I will try to have a look at the weekend. Is there a fixed date plan for release of 1.13.0

theoniko avatar Sep 15 '22 17:09 theoniko

@mvieth: I am quite busy these days. I will try to have a look at the weekend. Is there a fixed date plan for release of 1.13.0

Not a totally fixed date, but somewhat soon, the PCL 1.12.0 release was over a year ago. And we will definitely have to remove the deprecated stuff before the release. If you currently don't have much time, it's also fine if you remove everything but the stuff related to VTK and the VTK rendering backend (and revert the changes in pcl_find_vtk.cmake). As soon as this PR is in a good state, we can merge it, and someone else can remove the remaining things.

mvieth avatar Sep 15 '22 19:09 mvieth

@mvieth , @SunBlack In Cmakelist under visualization folder do i need to update anything to this line? At first look, i do not think so but could you confirm

theoniko avatar Sep 20 '22 17:09 theoniko

@mvieth , @SunBlack In Cmakelist under visualization folder do i need to update anything to this line? At first look, i do not think so but could you confirm

No, I would also say that this can stay as it is

mvieth avatar Sep 22 '22 08:09 mvieth

@mvieth , @SunBlack In Cmakelist under visualization folder do i need to update anything to this line? At first look, i do not think so but could you confirm

No, I would also say that this can stay as it is

Same here. The code looks like it is generally needed for VTK6 under 18.04 and has nothing to do with the legacy backend. You could test what happens if you remove it, but I think you can leave it as it is, since probably the support of VTK6 will be gone with the end of Ubuntu 18.04 next year anyway.

SunBlack avatar Sep 22 '22 10:09 SunBlack

Please also remove the deprecated function in bfgs.h

mvieth avatar Sep 25 '22 14:09 mvieth

Only a minor change, else looks good 👍

larshg avatar Sep 27 '22 04:09 larshg

Thanks for the contribution! 👍

larshg avatar Sep 29 '22 09:09 larshg