Skip to content
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

Proper tracks model: more robots #1048

Merged
merged 19 commits into from
Apr 14, 2022
Merged

Conversation

peci1
Copy link
Collaborator

@peci1 peci1 commented Jan 26, 2022

Adding proper tracks also to the following robots:

  • CCN_MARMOTTE
  • MARBLE_HD2
  • CSIRO_DATA61_DTR
  • CSIRO_DATA61_OZBOT_ATR
  • CORO_HD2

The battery draining is done via power_draining_topic battery plugin parameters.

I also fixed the Marble HD2 model textures. I've noticed Fuel converted them from PNG to JPEG between model versions 11 and 12, so I also did this change in the repo. I don't know what was the reason behind that change, though (the other teams' HD2 models still use PNG and there is no problem with it).

Coro HD2 had a smaller problem with the textures in repo SDF, so I fixed that, too.

See screenshots at peci1#3 .

Copy link
Member

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Only a couple of minor comments.

@@ -6,16 +6,17 @@
-->

<%
require_relative 'spawner'
#require_relative 'spawner'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is #require_relative 'spawner' not working?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does for coro_hd2_sc1, but not when this file is transitively included from coro_hd2_sc2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

<mu>0.273</mu>
<slip1>0.00165</slip1>
<mu>1</mu>
<slip1>0.002249</slip1>
<mu2>0.75</mu2>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the <mu2> needs to be adjusted for this vehicle. I tested 150, like you had for the other tracked vehicles, and it seems to work better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed and verified basic driving in the example world. The only remaining robot with different value is CSIRO DTR now (it has no mu or mu2 values specified). Should I also update it? At what occasion did the low mu2 pose a problem with the ATR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I can't seem to remember what wasn't working properly. I'm sure it had to do with being able to go over the pallet. However, I've tested with 150 and with 0.75 and they both seem to work fine. Having a higher mu2 makes sense though.

@@ -6,16 +6,17 @@
-->

<%
require_relative 'spawner'
#require_relative 'spawner'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it.

<mu>0.273</mu>
<slip1>0.00165</slip1>
<mu>1</mu>
<slip1>0.002249</slip1>
<mu2>0.75</mu2>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I can't seem to remember what wasn't working properly. I'm sure it had to do with being able to go over the pallet. However, I've tested with 150 and with 0.75 and they both seem to work fine. Having a higher mu2 makes sense though.

@azeey azeey merged commit 083c6a8 into osrf:master Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants