Hotfix: Add backwards compatibility for old scene serialization format#2986
Conversation
…e format The serialization format for the .scene files changed in moveit#2037. This commits a testcase using the old scene format. It will fail and a subsequent commit to introduce backwards compatibility to the scene-file parsing will make it pass.
…g new format, add old format test
…e version format This commit adds a mechanism for detecting the version of the scene file format to enable the loadGeometryFromStream method to read old version scene files without having to migrate them. To detect the version of the scene format, we use the content of the line following the start of an object: In the old version of the format, this specified the number of shapes in the object (a single int). In the new version of the format, it is the translational part of the pose of the object (i.e. three double values separated by spaces). To detect the format, we check for the number of spaces after trimming the string.
Codecov Report
@@ Coverage Diff @@
## master #2986 +/- ##
==========================================
+ Coverage 61.57% 61.59% +0.03%
==========================================
Files 370 370
Lines 33151 33162 +11
==========================================
+ Hits 20408 20424 +16
+ Misses 12743 12738 -5
Continue to review full report at Codecov.
|
…est using new format, add old format test" This reverts commit 0b68fab
rhaschke
left a comment
There was a problem hiding this comment.
Thanks for this contribution. I slightly simplified the code. Otherwise looks good to me.
Well done providing unit tests!
|
Fantastic, thank you very much for the swift review and improvements @rhaschke :-) |
simonschmeisser
left a comment
There was a problem hiding this comment.
I'd still highly welcome a PR for adding a sane format and I'm somewhat anxious about rebasing #2854 onto this ...
Thanks for pointing this out - taking a glance at #2854 it appears to change the serialization format as well - having the unit test should ensure that in the future backwards compatibility can be maintained more easily while still enabling new features to be added - the format has been released and people in the field are relying on it (if it requires changing a unit test to pass, it shouldn't be a patch release) Edit: Taking a closer look at #2854 - the changes appear to be backwards compatible by only appending to the end of an object and having logic to skip parts if unsupported / gracefully continue with the "current-new" format (not quite TLV, but still forwards compatible) - the issue fixed here was changing the format at the beginning of an object. @simonschmeisser thank you for your review. Looking forward to getting the fix merged and released to Noetic :-). |
|
Looks good to me. Really sorry for breaking the format. |
|
I strongly support migrating the file format to yaml. This still means asciifying floats, though. Do you have a binary format in mind, @v4hn? |
If you got a hammer, ... 😄
Not really, but do you foresee any problem with defining one ourselves?
I don't see the need to provide backward compatibility for files generated with a more current version of MoveIt, and that's the main downside I see for this style in contrast to yaml. If we can keep parsers for old versions around somehow, we might even use the binary serialization of the |
|
I think, having a human-readable and -editable format is required. I wouldn't go for a binary format. Actually, I'm surprised that you really had something like this in mind. We don't use binary formats anywhere else... |
|
The two of use agreed to disagree on this matter before. I simply prioritize accurate state representation of floats way more than the ability to edit files with a lot of internal structure in a text editor. There are of course merits to both approaches.
If you want to go for plain text approaches, an enhanced version of urdf might be an alternative to yaml, especially if we want to add tree structure to the planning scene at some point.
|
This PR adds backwards compatibility for the old scene serialization format to
PlanningScenefollowing #2037, cf. the regression report for Noetic in #2985. It first checks which version of the syntax is provided (by checking the line following the start of a new object - previously the number of shapes, now the translational component of the pose) and deactivates the subframe-poses if using the old format, as well as maintains compatibility by resetting the pose to identity.@felixvd @rhaschke Can you please review this PR, backport it to noetic-devel, and release? :-)
I've added unittests for the old format as well. Detail on changes/logic in the individual commits.
Resolves #2985