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

Support resource files with spaces #2877

Merged
merged 9 commits into from
Nov 21, 2020
Merged

Support resource files with spaces #2877

merged 9 commits into from
Nov 21, 2020

Conversation

chapulina
Copy link
Contributor

@chapulina chapulina commented Nov 11, 2020

Addresses #2870 .

Most file types were already working with whitespaces without issues. I added tests anyway to make sure they keep working. I've also only tested locally on Ubuntu, so I'm curious to see whether the new tests pass for all platforms.

Changes:

I also took the opportunity to add tests for entity names with spaces wherever I could. There are known issues with spaces in entity names that I didn't address though, such as creating transport topics or URIs (#2056).

The only file types that are not getting support for spaces are MTL files. See this comment: #2870 (comment). Technically we could update our fork of tinyobjloader, but it's not a good idea to diverge from upstream.


Manual testing

From a clone of this branch, try the following.

Load a world with spaces:

gazebo --verbose "test/worlds/world with spaces.world"

or

gazebo --verbose test/worlds/world\ with\ spaces.world

Load a model with spaces in various resources:

GAZEBO_MODEL_PATH=`pwd`/test/models/testdb gazebo --verbose

Then insert Model with spaces from the Insert menu.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina requested a review from iche033 November 17, 2020 01:40
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good. Also tested adding a sensor and light with space in the name and the model loads fine.

A few issues I noticed that can be addressed later if needed:

  • I see a few error msgs from ign-transport - looks like it does not allow spaces in the topic name:
Topic [/included model/joint_cmd] is not valid.
[Err] [JointController.cc:66] Error subscribing to topic [/included model/joint_cmd]
  • the plotting tool may not be working with spaces yet. It prints out a bunch of URI warnings, e.g.:
[Wrn] [URI.cc:327] Unable to parse URI [data://world/world with spaces/model/included model/joint/joint between links nested at different depths?p=axis/0/double/position]. Ignoring.
  • A couple of factory test failures that look related on homebrew:
FactoryTest.ActorSpaces
FactoryTest.FilenameFuelURL

test/models/testdb/model with spaces/model.sdf Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina
Copy link
Contributor Author

A few issues I noticed that can be addressed later if needed:

Yeah that's #2056. As I mentioned on the description above, this is out of scope for this PR. The focus here are spaces on the filenames. I took the opportunity to also test spaces in entity names wherever it was already working, but I'm not planning to fix the remaining issues in this PR.

FactoryTest.ActorSpaces

I also noticed that was flaky for me locally. The changes in f8fe4a3 seem to have fixed it.

FactoryTest.FilenameFuelURL

That looks flaky from before: https://build.osrfoundation.org/job/gazebo-ci-pr_any-homebrew-amd64/2206/testReport/(root)/FactoryTest/FilenameFuelURL/history/

@iche033
Copy link
Contributor

iche033 commented Nov 19, 2020

FactoryTest.FilenameFuelURL

ah it's the SSL certificate issue. This fixes the test for me:

@@ -1419,7 +1420,7 @@ TEST_F(FactoryTest, FilenameFuelURL)
 
   msgs::Factory msg;
   msg.set_sdf_filename(
-      "https://api.ignitionfuel.org/1.0/chapulina/models/Test box");
+      "https://fuel.ignitionrobotics.org/1.0/chapulina/models/Test box/1");

I had to include the version number in the URL otherwise the gazebo complains about not being able to find file in Test box/tip directory

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

looks good, just one minor style issue. We can address the FactoryTest.FilenameFuelURL issue here or separately. Either way is fine.

test/integration/factory.cc Outdated Show resolved Hide resolved
Signed-off-by: Louise Poubel <louise@openrobotics.org>
Signed-off-by: Louise Poubel <louise@openrobotics.org>
@chapulina chapulina merged commit 9ec3439 into gazebo9 Nov 21, 2020
@chapulina chapulina deleted the chapulina/9/spaces branch November 21, 2020 00:29
@chapulina chapulina mentioned this pull request Nov 23, 2020
12 tasks
chapulina added a commit that referenced this pull request Nov 23, 2020
Mainly adds tests to make sure we continue to support files with spaces.

Support encoded file names inside COLLADA files.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
chapulina added a commit that referenced this pull request Nov 25, 2020
Mainly adds tests to make sure we continue to support files with spaces.

Support encoded file names inside COLLADA files.

Signed-off-by: Louise Poubel <louise@openrobotics.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9 Gazebo 9 testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants