Skip to content

API cleanup/refactoring #476

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 21, 2015
Merged

API cleanup/refactoring #476

merged 8 commits into from
Nov 21, 2015

Conversation

xlz
Copy link
Member

@xlz xlz commented Nov 20, 2015

I was reviewing and writing up API documentation. It is time to minimize and shape API well before a major release. After 0.1, minor version will have to guarantee API compatibility and ABI compatibility for binary package release. Removing API after major release is very hard.

This PR removes packet processors from public APIs, and hides a few private functions from exported symbols. Exported symbols are greatly reduced.

I try my best to ensure no API breakage (ABI has to change). Reviews and suggestions are welcome. Each commit is properly functionally organized for review. It is possible more can be done to ensure future proof ABI compatibility.

Each commit is fully tested on Debian and Ubuntu. iai_kinect2 with ROS Indigo is also tested.

Interestingly this commit api: Work around setConfiguration in iai_kinect2 does not change ABI at all. Reverting it later would be safe.

@floe
Copy link
Contributor

floe commented Nov 21, 2015

Generally a good idea. I'm unsure if it would actually make sense to leave the internals of Registration exposed so people don't have to hack around this if they want to access the internal tables - I think that might be useful for some CV scenarios?

@xlz
Copy link
Member Author

xlz commented Nov 21, 2015

Exposing those tables directly makes it hard to maintain ABI compatibility. Though adding a function to return a pointer to one of them would be ok.

But I'm not sure of the usefulness of these tables. They are optimization cache of distort() and depth_to_color() and are hard to understand. If something here needs to be exposed, it can be distort() and depth_to_color() (But distort() + depth_to_color() is exactly apply()). I can add them. What do you think?

@floe
Copy link
Contributor

floe commented Nov 21, 2015

OK, I guess you're right, this is not something the average user needs to be bothered with. It's too specific to libfreenect2. So this can be merged as-is.

xlz added 8 commits November 21, 2015 15:01
@xlz
api: Move packet processor headers to internal
63d16e7 
File moving only.

Prepare to remove packet processor classes from public API.
@xlz
api: Remove packet processors from public API
f6e227d 
Packet processors should not appear in public API. Users never
directly interact with these classes.
@xlz
api: Add a function to configure depth processors
bad8fae 
Since direct access to depth processors is removed, add
Freenect2Device::setConfiguration() to allow users to
configure depth processors. This design is consistent with
IrCameraParams also being processed in Freenect2Device.
@xlz
api: Remove the abstract class PacketPipeline
1d516bc 
It is a useless duplicate of BasePacketPipeline.
@xlz
api: Hide protected function in Freenect2
0d92faa 
Freenect2 class is marked as API. A protected function in
Freenect2 got exported as a symbol.

Avoid that.
@xlz
api: Hide private functions in Registation
c74196b 
Registration class is marked as API. Private functions in
Registration got exported as symbols.

Avoid that.
@xlz
api: Work around setConfiguration in iai_kinect2
cdd4f06 
iai_kinect2 used p->getDepthPacketProcessor()->setConfiguration()
to configure the device. This is deprecated, but here provides
compatibility for such usage.
@xlz
cmake: add freenect2_INCLUDE_DIRS
611dae4 
iai_kinect2 expects this.
@xlz
Copy link
Member Author

xlz commented Nov 21, 2015

Rebased.

floe added a commit that referenced this pull request Nov 21, 2015
@floe
Merge pull request #476 from xlz/api-cleanup
25bf58a 
API cleanup/refactoring
@floe floe merged commit 25bf58a into OpenKinect:master Nov 21, 2015
@xlz xlz deleted the api-cleanup branch November 22, 2015 22:23
@xlz xlz mentioned this pull request Dec 2, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xlz @floe