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

Remove FZ-API #2747

Merged
merged 1 commit into from
Feb 15, 2019
Merged

Conversation

SunBlack
Copy link
Contributor

Solves #2701

@taketwo
Copy link
Member

taketwo commented Jan 10, 2019

As suggested in #2701 (comment), let's keep the grabber header around. Remove everything and just put a error pragma with an explanation what happened and perhaps a link to the GitHub issue where we discussed the removal. If somebody is affected indeed, this will be helpful.

@taketwo taketwo added module: io changelog: removal Meta-information for changelog generation labels Jan 10, 2019
@SunBlack
Copy link
Contributor Author

Can you explain, who you want to have implemented this error pragma?

E.g. for io/CMakeLists.txt:

if(WITH_FZAPI)
  message(FATAL_ERROR "FZ-API is not anymore supported and removed.").
endif()

Or keep most things in CMake and have
following class:

io/include/pcl/io/fotonic_grabber.h:

#pragma once

#error FZ-API is not anymore supported and removed.

//file end

@taketwo
Copy link
Member

taketwo commented Jan 10, 2019

I meant something similar to your second code snippet. However I'm not sure what you mean by "keep most things in CMake". I think it's fine to remove everything related to FZ from CMake and unconditionally install this header with #error.

The problem with the first option is that all our grabbers are ON by default. So hypothetical FZAPI user probably did not need to specify WITH_FZAPI explicitly.

@SunBlack
Copy link
Contributor Author

I added a commit to add error pragma as discussion based. Commits should squashed before merge.

The problem with the first option is that all our grabbers are ON by default. So hypothetical FZAPI user probably did not need to specify WITH_FZAPI explicitly.

Can we change default to OFF for this plugin?

@jspricke What do you think about the way error pragma is implemented?

@taketwo
Copy link
Member

taketwo commented Jan 10, 2019

Can we change default to OFF for this plugin?

I think we should remove it entirely, i.e. including this line:

PCL_ADD_GRABBER_DEPENDENCY("FZAPI" "Fotonic camera support")

@jspricke
Copy link
Member

If we remove all the sources, there is no need to add a #error, people will find that out themself ;). My proposal was rather to leave the sources in and only add the #error so if someone wants to maintain it, he can send a patch fixing bugs in the code and removing the #error.

@taketwo
Copy link
Member

taketwo commented Jan 11, 2019

If we remove all the sources, there is no need to add a #error, people will find that out themself ;).

Sure, they will find out themselves that the file is missing. But to understand why they will need to first check their CMake configuration to see if there is a mistake, then check Git history to find the commit that removed it, then find the PR/issue on GitHub with explanation. I think it would be nice if we spare them some time, by providing a pointer.

@SunBlack was eager to wipe out the sources for the following reasons:

This is like dead code: Mostly you don't care about it. But in case you are refactoring things (like we are currently doing. e.g. by partially replacing boost by std or CMake refactoring), you will always see this code and think short about it, before you see, it is dead code (e.g. if i search for a keyword with a general text search). So you spent even for dead code a bit time.

In case someone is enabling warning as errors (e.g. before submitting sth.) he needs always to disable everything with FZ-API, because we cannot fix compiler warnings there as long we don't see them. So whoever is still using this will have pain in the ass with this unmaintained code.

So if we are not removing the code, we can as well go through a proper deprecation procedure.

@SunBlack
Copy link
Contributor Author

So compared with current master:

  • Remove app/optronic_viewer files
  • Add #pragma error to fotonic_grabber.h
    ?

@taketwo
Copy link
Member

taketwo commented Jan 16, 2019

This is what I would do:

  • remove optronic_viewer
  • remove CMake options and finder scripts for FZAPI
  • remove all source files related to fotonic grabber except to "fotonic_grabber.h" header
    • install this header unconditionally
    • empty the header
    • add error pragma

Added a error pragma in case someone is still fotonic_grabber.
@SunBlack
Copy link
Contributor Author

New try. Hope this time everything is okay.

Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@SergioRAgostinho SergioRAgostinho merged commit c805b36 into PointCloudLibrary:master Feb 15, 2019
@SunBlack SunBlack deleted the remove_fzapi branch February 15, 2019 15:56
@taketwo taketwo mentioned this pull request Mar 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: removal Meta-information for changelog generation module: io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants