[reopen #274] Send Octomap diff instead of whole tree #302
Conversation
|
Comment from the original PR: This is essentially #255 by @TheBrewCrew with some minor fixes. The main reason is that in indigo we can only depend on OctoMap/octomap#103 and not OctoMap/octomap#85. This is handled in 28a6eb4. OctoMap/octomap#103 is already released as 1.6.9 in indigo (currently only shadow-fixed) and thus this can be merged once it is active. I have left my additions as individual commits, so it is clear for reviewing this, what I changed. The corresponding pull request is moveit_ros is moveit/moveit_ros#636. |
|
+1 |
| { | ||
| logInform("Cheaper to send tree instead of diff by %i bytes with %i changes", expected_size_diff-expected_size_tree, num_changes); | ||
| octomap_msgs::fullMapToMsg(*octree, scene_msg.world.octomap.octomap); | ||
| if(scene_msg.world.octomap.octomap.id != OCTOMAP_MSG_TYPE) { |
There was a problem hiding this comment.
curly brackets on new line
|
I wish we would all move away from deep changes like this in an LTS (indigo) release... |
| int expected_size_tree = octree->size()*(sizeof(float)+sizeof(char)); | ||
| if (expected_size_diff > expected_size_tree) | ||
| { | ||
| logInform("Cheaper to send tree instead of diff by %i bytes with %i changes", expected_size_diff-expected_size_tree, num_changes); |
There was a problem hiding this comment.
should this really be an inform? I would prefer debug.
Long-Term-Support means Stable API / Stable ABI. It does not mean "no new features". |
|
I agree it does not mean no new features, but given our lack of good unit tests and integration tests its very risky we'll break current user's setups - for example it seems like this week's changes are causing problems. I'm fine making mistakes, but it should be in the right branch. |
|
On Thu, Jul 21, 2016 at 01:18:03PM -0700, Dave Coleman wrote:
I agree. But in my opinion neither Btw: the message you referenced is not actually a problem of the indigo-devel branch. |
|
The right branch is technically the ROS-L branch ;-) |
|
On Fri, Jul 22, 2016 at 11:58:18AM -0700, Dave Coleman wrote:
I would prefer a plain "master" branch. |
|
it'd be great to get this merged in before friday's merge! |
|
@130s : Do you want to address the proposed changes before the merge? Otherwise I can re-open a PR based on this one. |
|
@dornhege thanks for the headsup and offering. Yes I appreciate if you could open a new PR. |
|
I can currently not test as moveit/moveit_ros#736 is preventing this from working. This might be resolved quickly, if it has something to do with my setup or is easily fixable. |
|
Continued in #322. |
@dornhege I've rebased #274 to include updates (esp. CI config for testing). Thanks.