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

Populating with new object models #242

Merged
merged 5 commits into from
Mar 25, 2022

Conversation

quarkytale
Copy link
Contributor

@quarkytale quarkytale commented Mar 22, 2022

Added all new models from DAVE Object Models and some more Cole's library.
Also, ported all new object models to Ignition fuel, checkout the collection here.

Updated world looks like this,

  • starting from top left we have the electrical(+bimanual) demo, hardhats, torpedos, sonobuoy, uxos
  • top right ocean rock
  • middle rexrov, mbari mars, flight data recorder, niskin, kelp, coral, fishes
  • bottom left mud anchor in mud pit, sunken ships, vases
    new_models_integrated

Please suggest any additions/modifications! You can run this by launching the integrated world roslaunch dave_demo_launch dave_integrated_demo.launch.

Also, @bbingham-nps since the models are uploaded, is it a good idea to delete them from the repo? Except the ones having inertial_calcs.py script, since that can't be added on Fuel.

Signed-off-by: Dharini Dutia <dharini@openrobotics.org>
@quarkytale quarkytale mentioned this pull request Mar 22, 2022
12 tasks
Copy link
Contributor

@mabelzhang mabelzhang left a comment

Choose a reason for hiding this comment

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

I haven't tried this. Just took a quick look.
For the models with the comments about inertial values not on Fuel - why do they need that comment? We should be able to upload the models with inertial values onto Fuel.

I had that comment on some of the models I added from Cole, because I couldn't replace the models owned by Cole. But if you're uploading these to your account, you should be able to just upload the models with the inertial values.

@bbingham-nps I'll add to the question for you in the PR description that, you had Python scripts for calculating the inertial values for each model. Those scripts are not on Ignition Fuel. I recommend keeping those Python scripts in this repo. I'll also mention that, the drawback of having these on Fuel is that, they're tied to an account, so if we want to change it (inertial values, etc) later after whoever uploads it leaves the project, it is easier to change it locally here in DAVE. For that reason, you may want to keep copies of those models in DAVE rather than deleting them, though that's duplication. If you choose to delete, you could always download from Fuel as needed in the future to make changes, and then re-upload it to a different account on Fuel - note that then the URL of the model will change. it's a preference question.

@quarkytale
Copy link
Contributor Author

@mabelzhang You're right, the ones I uploaded have inertial properties, I will remove the comments and use the Fuel links. Corrected the description, only models having inertial_calcs.py script needs to be preserved in the repo.

Signed-off-by: quarkytale <dharini@openrobotics.org>
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

A couple of big problems

  1. All the included models are not defined as static so they will end up dropping to the floor and worse, gazebo will constantly perform collision checking between these objects and the static seabed which significantly degrades performance of the simulation. As you can see in the image below, the Lionfish have crashed into the bed and the real time factor is 0.2 (I'm running this on a powerful 12 core machine)
dave-2022-03-24_16.36.25.mp4

image

I would say majority of these models should be static. Perhaps one or two torpedos and vases can be non-static for UUVs to pick them up. I would say we don't need to have models all over the simulation world. But we can have small cluster of objects beside each of the demo stations. For example, the objects near the mud pit are nice but I think two ships is redundant and instead we could have a few coral features and fish around the vases. Similarly, near the electrical panel, there is a bunch of hardhats that just fall to the seabed. These along with the torpedos could be better placed.
image

  1. Having blank spaces in fuel model names could lead to some issues. It will be especially problematic for other users who may want to download these models via scripts. They will need to explicitly replace the spaces with %20 characters for meaningful URLs. The model downloader will also download these models into folders with apostrophes around the name. See below
yadu@yadu:~/.ignition/fuel/fuel.ignitionrobotics.org/quarkytale/models$ ls
'flight data recorder'  'hardhat ribbed'    'hardhat superribbed'  'mud anchor'  'torpedo mk46'  'unexploded ordnance a'  'unexploded ordnance c'
'hardhat octagonal'     'hardhat standard'  'mbari mars'            sonobuoy     'torpedo mk48'  'unexploded ordnance b'

@Yadunund
Copy link
Member

@aaronchongth has a lot of experience uploading and managing models on fuel. He can guide you better on this if needed.

@aaronchongth
Copy link

aaronchongth commented Mar 24, 2022

Hey there! 👋 Pardon me for butting in,

We were plagued with these similar concerns around the start of the Open-RMF project, and has since found that the some best practices were needed to ensure consistent model loading/downloading behaviors over time. Most of the rules we end up adopting can be found here, naming consistencies, the absence of spaces, all referenced assets are within the model relatively, etc. The script I linked was just something random I was using a few years back, no idea if it still works.

absence of spaces

this prevented any cases of directories having apostrophes like @Yadunund mentioned, which allows us to easily write scripts to work with models en masse when needed, e.g. adding plugins, tweaking parameters, changing names, etc

naming consistencies between folder name, name in model.config and name in model.sdf

I haven't checked the models if they have differing names in different places yet, but in our experience this was rather important, as gazebo-classic references different names from different locations in different cases. Example in this screenshot of an innocent table,
image

Note that the directory name matches the name in model.sdf, but gazebo-classic displays the name in model.config when listing models available for spawning. Once loaded into the world and has duplicates, it opts for the name in model.sdf, it also adds underscores and indices 😅

image

This makes using world plugins to manipulate models quite tricky.

<uri>https://fuel.ignitionrobotics.org/1.0/quarkytale/models/MBARI MARS</uri>

We had to use a bunch of workarounds in the past when the gazebo-classic was not supporting fuel URLs, and this PR just showed us that it works now! 🥳 So all's good regarding using URLs.

uploading models

Could we also check if the assets and models created should be uploaded under the Open Robotics organization instead? I am unsure if we have any policy regarding this. I'm not sure if you need to be an admin, but you can choose under which user/organization to upload a certain model.

@bsb808
Copy link
Contributor

bsb808 commented Mar 24, 2022

Thank you all for the helpful discussion on all this

Deleting from repo
My preference, given the short time (1 week left) , is to leave them in the repo for now. That does introduce some redundancy, but I don't think we have time to properly understand the fuel integration. I fear we'll end up making a mess.

inertial_calc.py scripts
These are tiny little python scripts to estimate inertial values. I thought of them as self documentation of were the SDF values come from. If it allows for a cleaner repo, no reason to keep them around.

naming conventions
I love naming conventions and spaces/special characters in file/directory names are a pet peeve.

static objects
The following objects were intended to be non-static:
'flight data recorder' 'hardhat ribbed' 'hardhat superribbed' 'mud anchor' 'torpedo mk46' 'unexploded ordnance a' 'unexploded ordnance c'
'hardhat octagonal' 'hardhat standard' sonobuoy 'torpedo mk48' 'unexploded ordnance b'

The following objects can/should be static:
shipwreck, 'mbari mars'

Perhaps we can simplify the collision models for some of the non-static objects.

@quarkytale
Copy link
Contributor Author

quarkytale commented Mar 24, 2022

Thanks for the most valuable comments! I have some follow-up questions:

  • Currently, both the torpedos and sonobuoy are static, leaving the uxos non-static for manipulation. It did seem hazardous to put missiles near an electrical station, but I'm open to suggestions, maybe stationing them vertical for better visibility?
  • I put two versions of ships to highlight the distortion feature, I initially placed them one in each mud pit, or should I skip the non-distorted one?
  • Model names seems like a separate discussion? For DAVE object models I followed Cole's semantics. Changing the name will require deleting and re-uploading models and from them to have OpenRobotics as owner using admin access or Nate. I'll pass this on!

Apart from that working on making lionfish, uxos, mars and ships static plus adding more creatures near the mud pit vases.

@bsb808
Copy link
Contributor

bsb808 commented Mar 24, 2022

Currently, both the torpedos and sonobuoy are static, leaving the uxos non-static for manipulation. It did seem hazardous to put missiles near an electrical station, but I'm open to suggestions, maybe stationing them vertical for better visibility?

The torpedos and sonobuoys were library items as examples of things that we often pickup with underwater robots. Unrelated to the electrical station - the model collection world is just there to demonstrate the existence of various models.

I put two versions of ships to highlight the distortion feature, I initially placed them one in each mud pit, or should I skip the non-distorted one?

For the example, I think more is better ;)

@Yadunund
Copy link
Member

Apart from that working on making lionfish, hardhats, mars and ships static plus adding more creatures near the mud pit vases.

@quarkytale just to clarify, you only need to add <static>true</static> elements within the include blocks in the dave_integrated.world file. You don't need to change anything to the models on fuel.

Signed-off-by: quarkytale <dharini@openrobotics.org>
@quarkytale quarkytale requested a review from Yadunund March 25, 2022 05:52
Copy link
Member

@Yadunund Yadunund left a comment

Choose a reason for hiding this comment

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

I've made some tweak to improve performance and improve positioning of some models and UUVs b419074

@Yadunund Yadunund merged commit 72aeeeb into feature/integrated_world Mar 25, 2022
@Yadunund Yadunund deleted the integrated_world/new_models branch March 25, 2022 08:05
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.

5 participants