-
Notifications
You must be signed in to change notification settings - Fork 219
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
camera_info_manager: Adding public member function to manually set camera_info #20
camera_info_manager: Adding public member function to manually set camera_info #20
Conversation
* | ||
* @return true if new camera info is set | ||
* | ||
* @post @c cam_info_ updated, if valid; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess technically there's no validity check here. We could make sure the parameters are all reasonable, could potentially catch errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an API, it at least gives us the option to add various checks if a good reason for them arises.
… API (relies on ros-perception/image_common#20 ) ros_camera_utils: Adding CameraInfoManager to satisfy full ROS camera API (relies on ros-perception/image_common#20 )
… API (relies on ros-perception/image_common#20 ) ros_camera_utils: Adding CameraInfoManager to satisfy full ROS camera API (relies on ros-perception/image_common#20 )
ABI is compliant according to the abi-compliance-checker: https://rawgithub.com/jbohren/7990624/raw/eb8dd683bc63b7054194e0a85f66785e86ee3408/abi_compat_report.html |
Could this be merged? |
Sure. It still needs test cases. |
camera_info_manager: Adding public member function to manually set camera_info
I just noticed this was committed to groovy-devel. You want this released to Groovy? I thought we were talking about Hydro (and also Indigo). |
@vrabaud: I seem to have created a minor mess here. Although backwards compatible and ABI compatible, this is still an API change. I would rather not do that in Groovy. Please help me understand the branching scheme better. It looks like groovy-devel was last released to Groovy as 1.10.5 back in January, and that both Hydro and Indigo are using hydro-devel, but Hydro is at 1.11.1 and Indigo is at 1.11.2. Do we need or want different branches for different distros? Are we still doing new Groovy releases? |
Groovy is still released so we keep fixing. The API should not be broken so that PR should be reverted. We do a branch per distro if needed. Indigo and Hydro were the same (lately, there was a patch for Indigo only, that's why only the Indigo version got bumped). What was the motivation for changing the API ? I can split the patch in two and only change the API in Indigo (and thus create a new branch). |
Jon's patch does not break the API. It adds a new interface, which is needed for simulated camera support in Gazebo. I think it could be released in Groovy if needed there, but extending an API in an older, stable release is not normal practice. By that standard, Hydro would also be questionable. However, when there is a good enough reason, we can make an exception. @jbohren: do you need this released to Groovy? Hydro? |
Yeah sorry about forcing your had, @jack-oquin. I did actually mean (at this point) to merge this into Hydro. Merging into groovy now is unnecessary. I'd say we could just merge it into Indigo, but the fact that it doesn't run on 12.04 (which still blows my mind) means that I'd still have to use a fork of |
Jon: I am willing to move the change to hydro-devel and release it there on both Hydro and Indigo. Most people will not be using Indigo for quite a while yet, and I would prefer to keep them both in sync until a split becomes really necessary. I don't see any harm in releasing the other hydro-devel updates to Hydro as well as Indigo. @vrabaud: is that OK with you? Did I overlook anything? |
my bad, I had not seen that setCameraInfo has been moved, not deleted. So all's good for the API. hydro-dev is fine, and then please release for both hydro/indigo. |
Will do. Thanks for the assistance, Vincent. |
If I understand git correctly, reverting a previously merged commit could cause trouble if we were ever to merge groovy-devel into hydro-devel. I can't think of any reason why we would ever want to do that. |
image_common 1.11.3 released to Indigo including this API change. Docs updated. Hydro release to follow. |
Hydro release this morning. @jbohren: for code depending on this enhancement, you should specify the |
apt-get update before rosdep install
… API (relies on ros-perception/image_common#20 ) ros_camera_utils: Adding CameraInfoManager to satisfy full ROS camera API (relies on ros-perception/image_common#20 )
See #19
TODO before merge: