SteveMacenski / spatio_temporal_voxel_layer

A new voxel layer leveraging modern 3D graphics tools to modernize navigation environmental representations
http://wiki.ros.org/spatio_temporal_voxel_layer
GNU Lesser General Public License v2.1
644 stars 189 forks source link

Avoid compilation on jazzy or higher #301

Closed Rayman closed 2 months ago

Rayman commented 3 months ago

On jazzy the released openvdb version is already 10.0.1, so we don't have to compile the vendor package.

Compilation on the build farm was already quite flakey because the package is quite heavy, so I'd be best to avoid this. I don't know if openvdb is also released for ARM or any other ubuntu platforms, but we'll have to see on the buildfarm.

SteveMacenski commented 3 months ago

Do the released binaries on Jazzy / 24.04 work? They also existed on 20.04/22.04, but whoever actually did the distro releases added build flags that broke STVLs ability to use it.

Rayman commented 3 months ago

I only tested that the software compiles. Maybe I'll have time to do some more testing next week

SteveMacenski commented 3 months ago

That’s worth checking. It compiled in 20/22 as well, but didn’t work at runtime

Rayman commented 3 months ago

It seems like its working:

image

SteveMacenski commented 3 months ago

Oh that IS a good sign! What does the compute look like - in the range that you expect from Humble and prior? If so, this is absolutely the way to go for 24.04! We could (should?) remove the vendor package then as well so the build farm turns over and we don't waste builds that won't pass. Its still in the humble branch so if we ever needed to reintroduce it, its easy enough

Rayman commented 3 months ago

I don't have any numbers for CPU at the moment

We could remove the vendor package, but right now its functioning as a backwards compatibility wrapper. So you can leave the package in the buildfarm (it won't compile anything).

SteveMacenski commented 2 months ago

I don't see the point in using build resources for jazzy++ if the binaries work. As you've seen in the Discourse thread, this is causing problems so its worth removing it if we don't need it so we can get this released into jazzy and K-turtle on 24.04 without friction.

I won't remove it from the build farm for Humble / 22.04, but there is nothing working currently in 24.04 so its not removing what has never compiled in order to release :wink: Right now, every time I push it'll start trying to rebuild again. I can disable it in the release repository, but then the source makes it look like this is needed for ros2 / jazzy whereas it is not needed + is difficult to compile on Jetson devices. So, removing it I think is more clear to more passive on-lookers

I don't have any numbers for CPU at the moment

OK. Just ballpark it, I'm not looking for the difference in +/-3%, just general "yup, looks about the same". If there were a problem, you'd definitely see it in either the voxels not appearing or the compute resources skyrocketing.

Timple commented 2 months ago

I think @Rayman means keeping the vendor package but letting that find the binaries. So basically merge this PR. That won't consume any compilation cycles. Just a few file moves for the buildfarm.

SteveMacenski commented 2 months ago

Oh got it - I understand. Thanks for the clarification.

A sanity check on CPU I think is the last thing before we can merge + release, correct?

Rayman commented 2 months ago

Left: main branch, right: my PR. The flamechart seems about the same. htop also is about the same

image

SteveMacenski commented 2 months ago

The middle spike looks somewhat substantive but its had for me to tell from that graphic without units. What's the total CPU number look like in comparison? (i.e. is that the difference in 1%, 10%, 100% of a core)

Rayman commented 2 months ago

Total CPU seems about the same. The reason for the "spike" is that I could have slightly different compile settings, so there can be more/less inlining compared to the apt version.

htop on main: main

htop on PR: patch

SteveMacenski commented 2 months ago

0.3 & 0.1% is hardly a spike - that's well within the regime of noise

I'm happy with that. Anything else you want to do here? I can merge and release on Friday when I have my scheduled Nav2/STVL sync day

Rayman commented 2 months ago

I think we've done enough research. Let's get it merged!