Xml inertia rewriter #19463
Xml inertia rewriter #19463
Conversation
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
None of the application logic will be much different in C++ vs Python. So the question is which XML library does the best job of surgical edits to a human-readable XML document -- https://docs.python.org/3/library/xml.html or tinyxml2?
In the worst case, I'm pretty sure we can do what we need with either parser. (Identify the XML inertia element start and end characters and text-replace it directly within the file buffer as a string.) The question is whether either of the two parsers supports round-trip operation with minimal disruption to the file.
Our goal should be a minimal XML diff, where the only changes are to the <inertia>...</inertia> stanza, and nothing else. Unrelated whitespace and comments need to be unchanged.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
None of the application logic will be much different in C++ vs Python. So the question is which XML library does the best job of surgical edits to a human-readable XML document -- https://docs.python.org/3/library/xml.html or tinyxml2?
In the worst case, I'm pretty sure we can do what we need with either parser. (Identify the XML inertia element start and end characters and text-replace it directly within the file buffer as a string.) The question is whether either of the two parsers supports round-trip operation with minimal disruption to the file.
Our goal should be a minimal XML diff, where the only changes are to the
<inertia>...</inertia>stanza, and nothing else. Unrelated whitespace and comments need to be unchanged.
FWIW, etree didn't come even close to round-trip. I haven't researched all of the python std library alternatives; however lxml (external dependency we just recently deleted) does a good job.
Believe it or not (=P), I opted for the least-dependency-depth solution.
The only corner-cases of non-round-trip I've seen with tinyxml2 are white-space and quote-entity fiddling. I'm somewhat doubtful that any rewriter will not stumble over those (possible exception being lxml).
In any case, I don't much mind transcribing if we find a more precise round-trip library.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
The only corner-cases of non-round-trip I've seen with tinyxml2 are white-space and quote-entity fiddling. I'm somewhat doubtful that any rewriter will not stumble over those (possible exception being lxml).
In that case it sounds like we should use some library for xml parsing, but not use it for reserialization? Rather, we ask the library for the start+end character spans for the elements we intent to replace, edit the file-as-string buffer directly, and then write the string to disk instead of reserializing.
Since we do not distribute C++ binaries in releases, the def main() we tell users to call needs to be in Python. It would be plausible to write the implementation logic in C++ and bind it, so that main() is just a thin wrapper.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
The only corner-cases of non-round-trip I've seen with tinyxml2 are white-space and quote-entity fiddling. I'm somewhat doubtful that any rewriter will not stumble over those (possible exception being lxml).
In that case it sounds like we should use some library for xml parsing, but not use it for reserialization? Rather, we ask the library for the start+end character spans for the elements we intent to replace, edit the file-as-string buffer directly, and then write the string to disk instead of reserializing.
Since we do not distribute C++ binaries in releases, the
def main()we tell users to call needs to be in Python. It would be plausible to write the implementation logic in C++ and bind it, so thatmain()is just a thin wrapper.
Ah, good call on the distro issue. I was pondering splitting main/library anyway, just for testing access.
As for bit-perfect round-trip, I'll look into it more.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Ah, good call on the distro issue. I was pondering splitting main/library anyway, just for testing access.
As for bit-perfect round-trip, I'll look into it more.
A few more experiments have convinced me that if we want bit-exact round-trip, it's lxml or the highway.
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Hm. Even The entity-conversion thing seems pretty consistent across libraries I have tried. |
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
9cb2827
to
56a9b69
Compare
There was a problem hiding this comment.
r2 implements Sean's suggested simplified math, and updates some other commentary.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Rather, we ask the library for the start+end character spans for the elements we intent to replace
Looks like C (not C++!) expat can do this; I'm still looking at whether the std python expat bindings also expose enough to do the same thing.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Rather, we ask the library for the start+end character spans for the elements we intent to replace
Looks like C (not C++!) expat can do this; I'm still looking at whether the std python expat bindings also expose enough to do the same thing.
So, let's really think through what "preserve bit-exact input" really entails:
- using expat, I can write a call-back-based parse that builds up an index of the input file. It will still be rife with corner cases, owing to
<el></el>vs<el/>, and other nonsense. - I still have to infer something about indent style of the input file, since I will need to write out nested elements. If I see literal tabs in the input file, all bets are pretty much off.
- Even having succeeded at the above, there will still be difficulties preserving attribute order, etc.
Quoth @ggould-tri : "This is a terrible plan and you shouldn't do it."
What could/should we do?
- Guarantee preservation of semantic content, aside from the parts we edit
- Guarantee that any transformation is indempotent
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
So, let's really think through what "preserve bit-exact input" really entails:
- using expat, I can write a call-back-based parse that builds up an index of the input file. It will still be rife with corner cases, owing to
<el></el>vs<el/>, and other nonsense.- I still have to infer something about indent style of the input file, since I will need to write out nested elements. If I see literal tabs in the input file, all bets are pretty much off.
- Even having succeeded at the above, there will still be difficulties preserving attribute order, etc.
Quoth @ggould-tri : "This is a terrible plan and you shouldn't do it."
What could/should we do?
- Guarantee preservation of semantic content, aside from the parts we edit
- Guarantee that any transformation is indempotent
I don't see where the difficulty comes from. Maybe I've not correctly followed your explanation somehow.
I expect that trying to reserialize the file is impractical, and madness. I think we should use a real xml parser to find the file offsets that we need to edit (as well as the collision geometries to infer from), but then use string processing to do the edit and re-save -- no re-serialization.
For each <link>, if the file already has <inertia>...</inertia> then we presumably can find the starting and ending position of that inertia block (either as character offsets from the beginning of the file, or line + column offsets; they are equivalent). If there is no inertia stanza already, we can choose to put it as the first or last element within the enclosing <link>. Either way, we know exactly where it should go.
Taking the whole file as a single string, we can then erase the existing stanza (if any) and insert the new stanza charater-wise. There's no need to infer the tab-layout of the existing file. We can use a fixed indentation (e.g., +2 space per line). If the user wants to repave the file whitespace, they are invited to do so after we're finished.
Problem solved?
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I don't see where the difficulty comes from. Maybe I've not correctly followed your explanation somehow.
I expect that trying to reserialize the file is impractical, and madness. I think we should use a real xml parser to find the file offsets that we need to edit (as well as the collision geometries to infer from), but then use string processing to do the edit and re-save -- no re-serialization.
For each
<link>, if the file already has<inertia>...</inertia>then we presumably can find the starting and ending position of that inertia block (either as character offsets from the beginning of the file, or line + column offsets; they are equivalent). If there is no inertia stanza already, we can choose to put it as the first or last element within the enclosing<link>. Either way, we know exactly where it should go.Taking the whole file as a single string, we can then erase the existing stanza (if any) and insert the new stanza charater-wise. There's no need to infer the tab-layout of the existing file. We can use a fixed indentation (e.g., +2 space per line). If the user wants to repave the file whitespace, they are invited to do so after we're finished.
Problem solved?
What, exactly, about the already-implemented solution is madness? I don't see that there is a problem to be solved.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
What, exactly, about the already-implemented solution is madness? I don't see that there is a problem to be solved.
I hadn't looked at the code yet. I was only going by the above thread that was wondering about <el></el> idempotence and such. It seemed to me like any solution where that was in doubt was already barking up the wrong tree.
Does the current code meet the invariant that nothing except the text inside an inertia element is changed? Comments are intact, nothing is reflowed, no whitespace or indentation, changes, no " vs " swaps, etc. -- in other words, the diff -u between old and new file only mentions lines where inertia was edited (or inserted)?
If it already does that, then yes it was already solved. I can try it tomorrow on some sample files and see what happens.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I hadn't looked at the code yet. I was only going by the above thread that was wondering about
<el></el>idempotence and such. It seemed to me like any solution where that was in doubt was already barking up the wrong tree.Does the current code meet the invariant that nothing except the text inside an
inertiaelement is changed? Comments are intact, nothing is reflowed, no whitespace or indentation, changes, no"vs"swaps, etc. -- in other words, thediff -ubetween old and new file only mentions lines where inertia was edited (or inserted)?If it already does that, then yes it was already solved. I can try it tomorrow on some sample files and see what happens.
Please take it for a spin. We can spend as much effort as you deem necessary preserving non-semantic whitespace, but my estimation is that we are in a forest of only wrong trees (XML), and we need to get out alive.
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Please take it for a spin. We can spend as much effort as you deem necessary preserving non-semantic whitespace, but my estimation is that we are in a forest of only wrong trees (XML), and we need to get out alive.
While that is stewing, I'll see if I can prototype a pyexpat version that tries to preserve source text.
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
49ece11
to
486b88f
Compare
There was a problem hiding this comment.
Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
While that is stewing, I'll see if I can prototype a
pyexpatversion that tries to preserve source text.
r4 has the promised pyexpat prototype for side-by-side comparision. There is much still to fix there, but it sorta works, and gives a flavor of how to do it. The "infer indentation" code is madness; don't let that distract you.
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
515157f
to
2c7eb33
Compare
There was a problem hiding this comment.
Reviewable status: 3 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
r4has the promisedpyexpatprototype for side-by-side comparision. There is much still to fix there, but it sorta works, and gives a flavor of how to do it. The "infer indentation" code is madness; don't let that distract you.
r5 improves the prototype py version in various ways. I'd say at this point I mostly like it better than the .cc.
tmp/inertia_fixer.py line 58 at r5 (raw file):
return mapping def format_inertia(self, inertial_facts, fixed_inertia, indentation):
This formatter and the sdf one below will currently erase comments that were contained within the original inertial tag. Bug? Feature? So far I'm leaning feature.
tmp/inertia_fixer.py line 85 at r5 (raw file):
assert len(models) >= 1 # XXX This assertion is likely violated by files that use include tags. assert len(inertials) == plant.num_bodies() - 1, (
This sdf associator is all wrong and frequently breaks on probably-fine files found in drake. More work needed here.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
a discussion (no related file):
A corner case worth pondering:
bazel-bin/tmp/inertia_fixer_py --in_place examples/hydroelastic/python_nonconvex_mesh/pepper.sdf
inertial facts: ElementFacts(name='inertial', attributes={}, depth=3, start=SourceLocation(index=196, line=6, column=6), end=SourceLocation(index=515, line=17, column=6), parent=ElementFacts(name='link', attributes={'name': 'yellow_bell_pepper_no_stem'}, depth=2, start=SourceLocation(index=118, line=4, column=4), end=None, parent=None, serial_number=0), serial_number=None)
Traceback (most recent call last):
File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 376, in <module>
main()
File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 362, in main
output_text = processor.process()
File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 312, in process
maybe_inertia = self._maybe_fix_inertia(body_index)
File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 295, in _maybe_fix_inertia
M_GG_G_one = CalcSpatialInertia(inspector.GetShape(geom), 1.0)
RuntimeError: CalcMassProperties currently only supports .obj files for mesh geometries but was given '/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake_models/veggies/yellow_bell_pepper_no_stem_low.vtk'.
There was a problem hiding this comment.
Reviewable status: 4 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
A corner case worth pondering:
bazel-bin/tmp/inertia_fixer_py --in_place examples/hydroelastic/python_nonconvex_mesh/pepper.sdf inertial facts: ElementFacts(name='inertial', attributes={}, depth=3, start=SourceLocation(index=196, line=6, column=6), end=SourceLocation(index=515, line=17, column=6), parent=ElementFacts(name='link', attributes={'name': 'yellow_bell_pepper_no_stem'}, depth=2, start=SourceLocation(index=118, line=4, column=4), end=None, parent=None, serial_number=0), serial_number=None) Traceback (most recent call last): File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 376, in <module> main() File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 362, in main output_text = processor.process() File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 312, in process maybe_inertia = self._maybe_fix_inertia(body_index) File "/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake/tmp/inertia_fixer.py", line 295, in _maybe_fix_inertia M_GG_G_one = CalcSpatialInertia(inspector.GetShape(geom), 1.0) RuntimeError: CalcMassProperties currently only supports .obj files for mesh geometries but was given '/home/rico/checkout/drake/bazel-bin/tmp/inertia_fixer_py.runfiles/drake_models/veggies/yellow_bell_pepper_no_stem_low.vtk'.
For my part, I am OK with an error in this case, i.e., declaring that to be an unsupported input file. The inertia fixer is primarily aimed at stale model files from the internet past. If it bombs out on already-highly-drake-specific model files, I think it's still an improvement on the status quo.
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
2c7eb33
to
333d1e8
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
a discussion (no related file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
r5improves the prototype py version in various ways. I'd say at this point I mostly like it better than the .cc.
I ran it on drake/manipulation/models/iiwa_description/urdf/iiwa14_primitive_collision.urdf and drake/manipulation/models/iiwa_description/sdf/iiwa14_polytope_collision.sdf and I liked the diffs, other than the small nits posted below.
tmp/inertia_fixer.py line 58 at r5 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
This formatter and the sdf one below will currently erase comments that were contained within the original
inertialtag. Bug? Feature? So far I'm leaning feature.
I think it's a feature to repave <origin> and <inertia>, but maybe not <inertial>.
If the pre-existing <inertial> has a <mass> then I don't think we should repave the entire <inertial>, since we want to keep the existing mass intact and the existing mass element might have useful comments nearby.
tmp/inertia_fixer.py line 183 at r6 (raw file):
return f"""\ <inertial> {d(1)}<pose>0 0 0 0 0 0</pose>
BTW For both SDFormat and URDF, we will need to extract the canonical Bcm pose and emit it.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
tmp/inertia_fixer.py line 58 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I think it's a feature to repave
<origin>and<inertia>, but maybe not<inertial>.If the pre-existing
<inertial>has a<mass>then I don't think we should repave the entire<inertial>, since we want to keep the existing mass intact and the existing mass element might have useful comments nearby.
Mass is a bit tricky, because the mbp mass may not necessarily be the file mass. I'm sure there are things we can do.
FWIW, the only comments I have seen removed anywhere were these (below are just excerpts of longer paragraphs):
examples/kuka_iiwa_arm/models/desk/transcendesk55inch.sdf- TODO(Lucy-tri): Compute a more accurate spatial inertia of this table.
examples/kuka_iiwa_arm/models/table/extra_heavy_duty_table.sdf- TODO(lucy-tri): Compute a more accurate spatial inertia of this table.
examples/kuka_iiwa_arm/models/table/extra_heavy_duty_table_surface_only_collision.sdf- TODO(lucy-tri): Compute a more accurate spatial inertia of this table.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
tmp/inertia_fixer.py line 58 at r5 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
Mass is a bit tricky, because the mbp mass may not necessarily be the file mass. I'm sure there are things we can do.
FWIW, the only comments I have seen removed anywhere were these (below are just excerpts of longer paragraphs):
examples/kuka_iiwa_arm/models/desk/transcendesk55inch.sdf- TODO(Lucy-tri): Compute a more accurate spatial inertia of this table. examples/kuka_iiwa_arm/models/table/extra_heavy_duty_table.sdf- TODO(lucy-tri): Compute a more accurate spatial inertia of this table. examples/kuka_iiwa_arm/models/table/extra_heavy_duty_table_surface_only_collision.sdf- TODO(lucy-tri): Compute a more accurate spatial inertia of this table.
Ah. I forgot that sometimes we ignore the mass because its malformed. In those cases, it would be fine to repave <mass> as well.
I do think that we should still avoid repaving <inertial> (and probably <mass>) by default. I ran this on drake/manipulation/models/iiwa_description/urdf/iiwa14_primitive_collision.urdf and it deleted some probably-still-useful comments about the mass.
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
tmp/inertia_fixer.py line 183 at r6 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
BTW For both SDFormat and URDF, we will need to extract the canonical Bcm pose and emit it.
This will involve a line or two of math: the current summed-up inertia lives at body frame (so the individual geoms' inertias can be summed at all). It needs to be shifted back to the resulting center of mass.
Not on the table for the first PR is rotation to zero out products of inertia.
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
a6e6f59
to
fd5a2d8
Compare
There was a problem hiding this comment.
Reviewable status: 2 unresolved discussions, needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes
tmp/inertia_fixer.py line 58 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
Ah. I forgot that sometimes we ignore the mass because its malformed. In those cases, it would be fine to repave
<mass>as well.I do think that we should still avoid repaving
<inertial>(and probably<mass>) by default. I ran this ondrake/manipulation/models/iiwa_description/urdf/iiwa14_primitive_collision.urdfand it deleted some probably-still-useful comments about the mass.
So here (at some complexity cost) is a version that avoids murdering comments between {(pose|origin), mass, inertia}, but still murders comments within inertia (sdf only).
We can carry on this sort of pedantry to the n-th degree. I draw the line at attribute order.
…n. Sloppy code, but sorta works.
better association code as well.
rpoyner-tri
force-pushed
the
xml-inertia-rewriter
branch
from
eafb469
to
a8b0d59
Compare
There was a problem hiding this comment.
I've re-organized the files for testing and (eventual) distribution; these will turn up in a new PR, to avoid revision fatigue. Some final notes are added below to support continuity.
New PR is #19731.
Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers, labeled "do not merge", commits need curation (https://drake.mit.edu/reviewable.html#curated-commits), missing label for release notes (waiting on @rpoyner-tri)
tmp/inertia_fixer.py line 58 at r5 (raw file):
Previously, jwnimmer-tri (Jeremy Nimmer) wrote…
I'm still not a fan of trying to preserve attribute order.
Yes, I think its fair to repave any attributes within an element we're already repaving.
r8-r11 mostly take care of the missing child tags problem. The future PR should have tests to help sharpen things up further.
tmp/inertia_fixer.py line 183 at r6 (raw file):
Previously, rpoyner-tri (Rick Poyner (rico)) wrote…
This will involve a line or two of math: the current summed-up inertia lives at body frame (so the individual geoms' inertias can be summed at all). It needs to be shifted back to the resulting center of mass.
Not on the table for the first PR is rotation to zero out products of inertia.
Not addressed here; the new PR will have code and tests to achieve fully refined Bcm pose (shift to center of mass; rotate to zero out products of inertia).
Towards #18773.
This change is