search

ros-drivers / openni_camera

A ROS driver for OpenNI depth (+ RGB) cameras.
49 stars 60 forks source link

Consolidate rgbd repositories into a single repo #65

Closed 130s closed 6 years ago

130s commented 6 years ago

Issue by 130s Monday Apr 24, 2017 at 19:51 GMT _Originally opened as https://github.com/ros-drivers/openni_launch/issues/30_

The following repositories contain a single package. I suggest to create a new repo where we move all of these 3 packages to, in order to reduce the maintenance effort.

  1. openni_launch
  2. rgbd_launch
  3. openni_camera

Background Despite the inactive development, the release tasks for these packages still takes certain amount of work. For Lunar this time, I took charge of it and for total it took almost an hour, mainly because these packages are inter-depended and I had to make release one by one and wait for the binary to become ready, in order for bloom to be able to resolve the necessary dependency.

Suggested solution Create a new repo and move all of these 3 packages there. Suggestion for the repo name appreciated. Here I temporarily use openni_meta.

New repo and the contents (being updated over time taking suggestions in):

ros-drivers/openni_meta
|- freenect_camera
|- freenect_launch
|- openni_camera
|- openni_launch
|- openni_meta
|- rgbd_launch

openni_meta is a meta package that run_depends on these 3 packages.

130s commented 6 years ago

Comment by piyushk Monday Apr 24, 2017 at 20:53 GMT

I understand the issues with extra maintenance effort. If you would prefer, you can also merge freenect_* into a meta repo as well.

I also recommend calling the repository rgbd_meta.

130s commented 6 years ago

Comment by 130s Monday Apr 24, 2017 at 21:11 GMT

@piyushk I'm only doing the release tasks for these packages so feedback from actual developers like you is appreciated.

Do you mean to include freenect_* into the same single repo where openni_* and rgbd_* are, and that repo name can be rgbd_meta?

130s commented 6 years ago

Comment by piyushk Monday Apr 24, 2017 at 21:21 GMT

@130s Thanks for spending time releasing!

Yes, that is exactly what I meant!

130s commented 6 years ago

Comment by 130s Tuesday Apr 25, 2017 at 01:21 GMT

I see.

@piyushk could you provide one more feedback? A caveat I can think of by the unified release from the aggregated repo is that all packages need to align the same package version. The current ersions taken from the main branch of each repo:

I propose either:

130s commented 6 years ago

Comment by piyushk Tuesday Apr 25, 2017 at 01:34 GMT

That's the only repository. There are multiple packages inside it.

I would recommend 2.1.4 across all of them, since this change maintains backward compatibility. However, given that other numbers are going to change, 2.2.0 sounds good to me.

130s commented 6 years ago

Comment by mintar Tuesday May 30, 2017 at 11:50 GMT

What about openni2_camera and openni2_launch? openni2_launch also depends on rgbd_launch, so by that logic it should also go into that merged repo.

Personally, I'm not convinced of the proposal to merge everything together. Especially openni2_camera, openni_camera and freenect_stack don't seem to have any inter-dependencies (please correct me if I'm wrong). It might be more convenient to release everything in one go, but I think this is outweighed by the loss of clarity when trying to pinpoint a bug in one mega-repository.

Also, there are third-party packages for cameras such as the Orbbec Astra that include rgbd_launch. It's a bit weird for them to require the freenect, openni and openni2 libraries just to include some launch file.

Just my 2 cents, it's up to you maintainers to decide. :)

130s commented 6 years ago

Issue mover script ended in the middle so not all comments were moved. I don't know why. I completed the original ticket anyway, so closing.