Fix parsing 'type' attibutes in plugins

[chapulina]

🦟 Bug fix

• Critical bug identified in Bumps in garden: use ign-math7 and dependents gz-sim#1264
• Bug introduced in Support printing sdf poses in degrees and allow snapping to commonly used angles #689

Summary

The logic added to check for the type attribute has broken use-cases inside ign-gazebo, such as this:

    <plugin filename="ignition-gazebo-triggered-publisher-system" name="ignition::gazebo::systems::TriggeredPublisher">
      <input type="ignition.msgs.Empty" topic="/in_1"/>
      <output type="ignition.msgs.Boolean" topic="/out_1">
          data: true
      </output>
    </plugin>

Which now causes an assertion:

173: Error [Param.cc:768] Unknown parameter type[ignition.msgs.Boolean]
173: Exception [Param.cc:81] SDF ASSERTION                     
173: Invalid parameter
173: In function       : Param
173: Assert expression : this->dataPtr->ValueFromStringImpl( this->dataPtr->typeName, _default, this->dataPtr->defaultValue)

My assumption from #689 is that we only want to parse the /pose@type attributes here, but don't affect the rest, so I added the extra check for "pose". I left a TODO there because I think we should remove the special type handling from that function to a pose-specific function.

I suspect I may be missing some important detail, but tests pass. So I hope this is enough as a band-aid to fix the regression, and I'd like to check with @aaronchongth and @azeey when they're back on making a proper fix.

Checklist

• Signed all commits for DCO
• Added tests
• Updated documentation (as needed)
• Updated migration guide (as needed)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸🔸

[scpeters]

I think the problem is related to unrecognized type values, like ignition.msgs.Boolean in triggered_publisher.sdf

[chapulina]

I was just fixing the test failure. With my change, the parser is now converting false into 0.

I haven't touched the parser much before, so I'd appreciate some guidance on the proper fix. My assumption from #689 is that we only want to parse the /pose@type attributes here, but don't affect the rest, that's why I added the extra check for "pose" with a TODO to remove the special type handling from that function to a pose-specific function. But I suspect I may be missing some important detail.

[codecov-commenter]

Codecov Report

Merging #809 (27fc0a4) into sdf12 (8785b7a) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##            sdf12     #809      +/-   ##
==========================================
- Coverage   90.70%   90.70%   -0.01%     
==========================================
  Files          78       78              
  Lines       12437    12432       -5     
==========================================
- Hits        11281    11276       -5     
  Misses       1156     1156              

Impacted Files	Coverage Δ
src/Param.cc	91.29% <100.00%> (+0.01%)	⬆️
src/parser.cc	87.48% <100.00%> (-0.07%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8785b7a...27fc0a4. Read the comment docs.

[chapulina]

All tests are passing here, so I'm opening this for review. I updated the PR description with more context.

[chapulina]

• The fix was also confirmed downstream: Build SDF from source to reproduce test failures gz-sim#1266 (comment)

[scpeters]

I've added a line in 5fc1571 that reproduces the failure seen in ign-gazebo, caused by an unrecognized type attribute

[scpeters]

I'd like to see what @aaronchongth thinks about this change

[chapulina]

I've added a line in 5fc1571 that reproduces the failure seen in ign-gazebo, caused by an unrecognized type attribute

It failed similar to the other line:

-    <property type='Ignition.Msgs.Boolean'>false</property>
+    <property type='Ignition.Msgs.Boolean'>0</property>

I'd like to see what @aaronchongth thinks about this change

My intention here is to revert the regression without reverting #689 completely. I don't think this is the final fix, and I hope the original authors come back with a cleaner fix later. I'd just like to fix the broken use cases on ign-gazebo for from-source Fortress users and source/nightly Garden users asap.

Are you concerned we may be breaking something else with this, @scpeters ?

[aaronchongth]

Hi @chapulina, @scpeters! Thanks for raising this so quickly. My bad for assuming that all the types will still be consistent with what sdformat12 is used to, I guess I didn't quite understand what "custom" means 🤦

The changes to parser.cc in #689, especially checking for the type attribute, was added specifically for this scenario in Plugin_TEST.cc. So there should not be any worry regarding breaking other behaviors within sdformat12.

I have a fix ready #811, let me know what you think. I haven't managed to run this fix against ign-gazebo6 yet as it builds pretty slowly on my available laptop, but I should get some results in a few hours when it's done.

[osrf-triage]

This pull request has been mentioned on Gazebo Community. There might be relevant details there:

https://community.gazebosim.org/t/new-ignition-releases-2022-04-13-fortress-edifice/1367/1