Fix usage of relative paths with environment variables

[chapulina]

I noticed that some tests started failing since #2765, such as UNIT_Actor_TEST:

UNIT_Actor_TEST failure

251: [ RUN      ] ActorTest.Load
251: [Msg] Waiting for master.
251: [Msg] Connected to gazebo master @ http://127.0.0.1:11345
251: [Msg] Publicized address: 172.17.0.2
251: [Wrn] [SystemPaths.cc:459] File or path does not exist ["/var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/worlds/walk.dae"] [/var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/worlds/walk.dae]
251: [Err] [MeshManager.cc:211] Unable to find file[/var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/worlds/walk.dae]
251: [Wrn] [Actor.cc:177] Couldn't find mesh [/var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/worlds/walk.dae]. Not loading skin.
251: [Dbg] [ServerFixture.cc:203] ServerFixture load in 0.7 seconds, timeout after 600 seconds
251: /var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/gazebo/physics/Actor_TEST.cc:56: Failure
251: Value of: skelAnims.empty()
251:   Actual: true
251: Expected: false
251: /var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/gazebo/physics/Actor_TEST.cc:57: Failure
251: Expected equality of these values:
251:   skelAnims.size()
251:     Which is: 0
251:   1u
251:     Which is: 1
251: /var/lib/jenkins/workspace/gazebo-ci-gazebo11-bionic-amd64-gpu-nvidia/gazebo/gazebo/physics/Actor_TEST.cc:58: Failure
251: Value of: skelAnims["walking"] != nullptr
251:   Actual: false
251: Expected: true
251: [Dbg] [ServerFixture.cc:129] ServerFixture::Unload
251: [Wrn] [Publisher.cc:136] Queue limit reached for topic /gazebo/default/pose/local/info, deleting message. This warning is printed only once.
251: [  FAILED  ] ActorTest.Load (836 ms)

That's a use case for relative paths that I overlooked on the other pull request. Resources could be found with paths relative to environment variables like GAZEBO_RESOURCE_PATH and GAZEBO_MODEL_PATH. But when I added the conversion to full paths relative to the SDF file, I didn't give Gazebo a chance to also look for the files relative to those environment variables.

This PR changes the asFullPath function to only return the full path in case it exists on disk. This allows the search to continue using the environment variable in case there's no matching file relative to the SDF.

I changed some worlds to more robust URIs, but left some others to make sure those use cases still work.

[j-rivero]

This PR changes the asFullPath function to only return the full path in case it exists on disk. This allows the search to continue using the environment variable in case there's no matching file relative to the SDF.

If I understand this correctly, the priority of the paths to search would change from previous releases, where environment variables + relative names where the only option and now it will check for absolute path and afterwards will fallback to the previous behavior. Is this correct? In that case, I think we could find some regressions due to some scenarios where resources use the same name. Changing the order of checking would make sense?

[chapulina]

If I understand this correctly, the priority of the paths to search would change from previous releases, where environment variables + relative names where the only option and now it will check for absolute path and afterwards will fallback to the previous behavior. Is this correct?

So, this is the situation:

• Until 11.1.0: relative paths could only be resolved against environment variables
• On 11.2.0: relative paths loaded from SDF files can only be resolved against the SDF file - the previous use case was broken
• From 11.3.0 with this PR: relative paths will first be resolved against the SDF file, and if that fails, fallback to the environment variables

Changing the order of checking would make sense?

I'd have to think of a way of doing this, it wouldn't be trivial with the current architecture. But in any case, I think that checking relative to the file first is the behavior that users may expect, and it follows the order explained in the Finding resources design document.

[j-rivero]

Changing the order of checking would make sense?

I'd have to think of a way of doing this, it wouldn't be trivial with the current architecture. But in any case, I think that checking relative to the file first is the behavior that users may expect, and it follows the order explained in the Finding resources design document.

Sounds good to me. The problem of fixing behavior/features in minor releases is that can change current results from previous versions of the same major serie. The PR solves the situation introduced in 11.2.0 which is the important part of it. I still see that 11.3.0 could generate conflict scenarios that in 11.1.0 did not exist but if the solution is not trivial, I'm ok assuming corner cases that should be fixed anyway for the future to respect design doc. Does make sense to add something to Migration.md about this?

[j-rivero]

It's hard to detect failures related these specific changes in the set of failing tests but I can not detect any regression looking at the latest gazebo11 branch CI builds.

[chapulina]

Does make sense to add something to Migration.md about this?

Yeah I'll add a note as I merge to prevent CI being retriggered here 👍