3 -> 4

[iche033]

➡️ Forward port

Port ign-sensors3 to ign-sensors4

Branch comparison: ign-sensors4...ign-sensors3

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

[chapulina]

This failed consistently on all GitHub actions:

  [ RUN      ] GpuLidarSensor/GpuLidarSensorTest.TestThreeBoxes/ogre2
  [Msg] Loading plugin [ignition-rendering-ogre2]
  [Msg] Publishing laser scans on [/ignition/sensors/test/lidar1]
  [Msg] Publishing laser scans on [/ignition/sensors/test/lidar2]
  /github/workspace/test/integration/gpu_lidar_sensor_plugin.cc:540: Failure
  The difference between sensor1->Range(0) and expectedRangeAtMidPointBox2 is 0.00010395050048828125, which exceeds 1e-4, where
  sensor1->Range(0) evaluates to 4.5001039505004883,
  expectedRangeAtMidPointBox2 evaluates to 4.5, and
  1e-4 evaluates to 0.0001.
  [  FAILED  ] GpuLidarSensor/GpuLidarSensorTest.TestThreeBoxes/ogre2, where GetParam() = "ogre2" (309 ms)

Is this on your radar, @iche033 ? Should we make those checks not required for ign-sensors4?

[iche033]

hmm the test was failing in different checks on macOs as mentioned in #66. I don't remember if it was happening on github actions though. How about we make them not required for now and add info on the new failed checks to issue #66?

[chapulina]

It looks like Actions has been consistently green for ign-sensors4 until now: https://github.com/ignitionrobotics/ign-sensors/actions/workflows/ci.yml?query=branch%3Aign-sensors4

I don't think this PR is introducing the issue though. Maybe an upstream change? I retriggered the ign-sensors4 branch to see if it changes: https://github.com/ignitionrobotics/ign-sensors/actions/runs/950365124

[iche033]

after taking a closer look at the test failure, it's caused by loss of precision. The expected vs actual depth value now slightly exceeds the specified tolerance (1e-4). The only upstream PR that looks related is gazebosim/gz-rendering#296 that configures the depth texture resolution based on the number of lidar samples instead of a hardcoded 1024 value (to save memory). I did not expect this to affect precision though since depth buffers precision is mainly affected by near and far clip planes which had not changed. I reverted gazebosim/gz-rendering#296 locally and verified that depth precision didn't change so the test is passing locally as well.

I think we can do one of the followings:

• revert Reduce lidar data discretization gz-rendering#296 and rerun ign-sensors test on github actions (I'm not sure how to do that with a custom upstream branch)
• relax tolerance for that particular check from 1e-4 to 2e-4 to make the test pass.

[iche033]

I reverted gazebosim/gz-rendering#296 locally and verified that depth precision didn't change so the test is passing locally as well.

Spoke too soon! The value did change by a very small amount locally, by a diff of 6-e5, . So I think gazebosim/gz-rendering#296 is the reason for the test failure. So the tradeoff here is depth sensor accuracy vs gpu memory. The precision loss is very small so I'm leaning towards just relaxing the tolerance for that particular check in the gpu integration test

[chapulina]

I'm leaning towards just relaxing the tolerance for that particular check in the gpu integration test

SGTM 👍

[codecov]

Codecov Report

Merging #138 (79e1a36) into ign-sensors4 (dcc7b85) will increase coverage by 0.23%.
The diff coverage is 85.29%.

❗ Current head 79e1a36 differs from pull request most recent head c087c9a. Consider uploading reports for the commit c087c9a to get more accurate results

@@               Coverage Diff                @@
##           ign-sensors4     #138      +/-   ##
================================================
+ Coverage         75.86%   76.10%   +0.23%     
================================================
  Files                23       23              
  Lines              2329     2390      +61     
================================================
+ Hits               1767     1819      +52     
- Misses              562      571       +9     

Impacted Files	Coverage Δ
src/Lidar.cc	70.83% <0.00%> (-0.38%)	⬇️
src/ThermalCameraSensor.cc	60.00% <87.50%> (+3.29%)	⬆️
src/Sensor.cc	88.00% <88.23%> (-0.30%)	⬇️

---

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 a9ba28d...c087c9a. Read the comment docs.

[iche033]

CI green after c087c9a