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

fixing missing DISABLE_VTK on recent PCL versions #871

Merged
merged 1 commit into from
May 30, 2022

Conversation

matlabbe
Copy link
Member

No description provided.

@matlabbe matlabbe changed the title fixing missing DISABLE_VTK on recent PCL versions: #618 #790 #795 fixing missing DISABLE_VTK on recent PCL versions May 28, 2022
@windelbouwman
Copy link
Contributor

This surely would fix building with newer version of the PCL library, so this is a good change.

Still, I believe the code could be improved even further by not relying on the DISABLE_VTK define at all in the rtabmap code, since that define is coming from the PCL library (it belongs to PCL, not rtabmap code), so it might change in the future. Instead, it might be good to create an own define, something like RTABMAP_WITH_VTK, to indicate that rtabmap will be build using VTK.

Anyway, good change, this will solve compilation issues!

@matlabbe matlabbe merged commit 8f8256c into master May 30, 2022
@matlabbe matlabbe deleted the remove_disable_vtk branch May 30, 2022 11:24
@matlabbe
Copy link
Member Author

I agree, this was a quick fix (quite busy right now). Defining RTABMAP_WITH_VTK for c++ based on VTK_FOUND cmake variable would be better.

I think it would work in thoses cases:

  • PCL built with VTK, rtabmap built with Qt
  • PCL built with VTK, rtabmap not built with Qt
  • PCL not built with VTK, rtabmap built without Qt
  • PCL not built with VTK, rtabmap built with Qt: this is the case I thought DISABLE_VTK was needed, but this should correctly fail by cmake because pcl visualization component won't be found.

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

2 participants