-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Improve lidar performance #4505
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a big performance improvement, thanks a lot for this!
Unreal/Plugins/AirSim/Source/UnrealSensors/UnrealLidarSensor.cpp
Outdated
Show resolved
Hide resolved
Unreal/Plugins/AirSim/Source/UnrealSensors/UnrealLidarSensor.cpp
Outdated
Show resolved
Hide resolved
Unreal/Plugins/AirSim/Source/UnrealSensors/UnrealLidarSensor.cpp
Outdated
Show resolved
Hide resolved
Some more things which might also help in extracting more performance - AirSim/Unreal/Plugins/AirSim/Source/UnrealSensors/UnrealLidarSensor.cpp Lines 139 to 141 in 19ac0f2
This is just getting the first component with id != -1, instead of continuing to iterate, we can break early -
AirSim/Unreal/Plugins/AirSim/Source/UnrealSensors/UnrealLidarSensor.cpp Lines 105 to 107 in 19ac0f2
Here
This is a string comparison in the fast path,
This could be made a const ref |
Another thing which I just wanted to note, from my (limited) experience with parallel programming, it was often better to have lesser number of parallel tasks each doing more work, rather than large number of small tasks. This helped in reducing core contention and improving cache locality as well. |
- change params to const
Thanks @rajat2004! |
Since this project is going to be archived, could this pull request be merged in the Colosseum fork? |
Hey @wouter-heerwegh, I'm happy to merge this in. Give me a minute to figure out how to pull this PR into the forked repo and I will. |
Adding this to Colosseum via PR #1 |
Fixes
Fixes: #4474
About
Lidar raycast make the calculation using line trace on single thread.
This PR take advantage of
ParallelFor
to improve the performance.How Has This Been Tested?
Lidar config tested
Benchmark:
Before:
After:
You can see the execution time is about 8 times faster, which obviously depends on your CPU.
UPDATE:
After some tests, I have found the point cloud generated by the multithread method is not the same as the original.
Fixed here 497cf06
Also, it's pretty complicated to verify that the clouds are exactly the same so I have added a test to check they are identical.
I ran both the original method the and new method, compared the point cloud arrays and made sure I didn't hit the breakpoint in case they are different.
To test it yourself, you can override the
UnrealLidarSensor::getPointCloud
method with the followingUnrealLidarSensor::getPointCloud