Fix C++ client request wedge (uninitialized flag) and depth packing (raw fp16 meters, 16FC1)

[andrewjong]

About

Two fixes found while integrating the ROS2 C++ bridge into a mapping pipeline on Linux (Blocks + custom environments, UE 5.7.4). (Scoped down per review: the clock/header-stamping work was removed from this PR — deferring that area to #182 and the planned sim-published clock topic.)

1. cpp client: initialize fis_canceled_ — TAsyncResultProviderBase's constructor initializer list skips fis_canceled_, leaving it uninitialized memory. Both client worker threads consult FIsCanceled(): when the garbage reads true, RequestSendingThreadProc silently skips sending the request and ResponseReceivingThreadProc pops the pending-response entry without ever calling SetDone, so the caller's Wait() blocks forever — and every later request queues behind it. In practice this permanently wedges the C++ client's whole request channel (the ROS2 bridge's /clock never publishes, move_*/SetPose service calls hang), nondeterministically depending on heap state. One-line fix.

2. depth: fix packing — transmit raw float16 meters bit-exactly (encoding 16FC1) — the depth materials write meters into the fp16 render target, but the packing did static_cast<uint16>(meters) while declaring the wire as 16UC1 millimeters: consumers received depth quantized to whole meters (a camera 3.24 m from a wall read raw 3), and sky/no-hit pixels wrapped around into phantom finite depths. Per review discussion, the wire now carries each pixel's FFloat16 bit pattern verbatim (little-endian, new encoding label 16FC1, meters): the packing loop is a pure bit copy — no value conversion, no added quantization, no 65 m range cap — and sky arrives as +inf. Downstream decoders updated in the same commit (ROS2 C++ bridge → standard 32FC1 float meters with non-finite → NaN; legacy Python rosbridge; Python client unpack_image + examples), with 16UC1 branches kept for older sims. The label change is deliberate so older clients fail loudly rather than misread fp16 bits as integers.

How Has This Been Tested?

Linux (Ubuntu 22.04), Blocks built from this repo at current main with UE 5.7.4, ROS 2 Humble; the bridge node driving camera + non-physics robot scenes in Blocks and several custom UE environments.

• Fix 1: Before — the bridge's /clock never published and all RawRequest/SetPose service calls hung indefinitely; a gdb thread dump showed the rclcpp executor blocked in Client::Request → AsyncResult::Wait() with both client worker threads idle and empty queues (entry consumed without completion via the FIsCanceled() path). The Python client against the same sim worked, which localized the fault. After — /clock publishes at a steady 50 Hz, SetPose round-trips return success=True, and a downstream RGB-D mapping pipeline has run extended sessions with no request wedges.
• Fix 2: Before — raw depth read median 3 for a camera ~3.2 m from the scene (whole meters). After — the bridge's decoded 32FC1 stream shows fractional-millimeter depth structure (median 6.4648 m on one test view) and far geometry at ~4 km, both impossible under the previous packing; sky pixels arrive as NaN after the bridge's non-finite mapping.

Screenshots and videos (if appropriate):

N/A — behavioral fixes; measurable evidence described above.

🤖 Generated with Claude Code

[jonyMarino]

We must indicate that the output is in meters, not millimeters. Multiplying by 1000 is unnecessary and adds error.

[andrewjong]

Thanks! Hm this is what Claude Fable said, thoughts?

I'd like to push back gently on this one. The R channel arrives as fp16 meters, so its precision floor is already ~4 mm at 4–8 m and ~31 mm at 32–64 m — the ×1000 multiply happens in float32 and adds at most 1 ulp on top of that, while the uint16-mm representation quantizes at 1 mm, i.e. below the fp16 floor. Keeping raw meters in a uint16 instead quantizes depth to whole meters (before this patch, a camera 3.24 m from a wall read raw 3), which makes the stream unusable for RGB-D consumers. Millimeters is also what this function already documents (// ...depth in mm, DepthMilli) and matches the standard ROS 16UC1 depth convention. If the concern is the 65.535 m range cap for long-range aerial use, I'm happy to make the scale configurable per capture (defaulting to mm), or to implement pixels-as-float for true float32 meters — let me know which you'd prefer.

[andrewjong]

Also:

And 16UC1 depth in millimeters is the established ROS/OpenNI/RealSense convention; relabeling ProjectAirSim's 16UC1 as integer meters would be a surprising contract for every downstream ROS consumer.

[LucasJSch]

Currently there's no float32 computation in the DepthMili variable. It's all with uint16.

Regarding the second comment: It's an interesting observation, but the most performant solution would be to compute that at a later stage.

[andrewjong]

Sorry for forwarding these responses through Claude! I'm still familiarizing with the code base, just trying to contribute what helped me resolve a bug during my usage. From what I understand though this seems to make sense. I think the main issue if we use meters here is the loss of precision for anything sub-meter? Please let me know if I'm misunderstanding!

Thanks for taking a look! Small clarification on the code: On "there's no float32 computation in DepthMilli, it's all uint16" — that's just not what the code says, in either version. The original line is:

float DepthMilli = SrcPixel.R.GetFloat();      // float32, from the fp16 texel
uint16 DepthUint16 = static_cast<uint16>(DepthMilli);

DepthMilli is declared float, and FFloat16::GetFloat() returns float32. The value lives as float32 right up to the final cast — in both the original and the patch. All the patch adds is one float32 multiply before that cast. So the premise that the multiply somehow happens "in uint16" (and would therefore add integer error) is a misreading; the multiply is exact to 1 ulp in float32, then a single quantization to uint16 happens either way. The only question is what one uint16 step means — 1 mm (patch) or 1 m (original).

For second comment:

Ah, I see — agreed that core_sim shouldn't adopt conventions because they're ROS conventions, and the ROS bridge is the right place for ROS-facing conversion. I only cited 16UC1-mm as corroboration; the actual reason for the ×1000 is internal to the sim: the wire is uint16, so a later stage can only convert what survives the cast, and integer meters destroy sub-meter precision at packing time (3.0 m and 3.9 m both arrive as 3 — nothing left for the bridge to compute). Any sub-meter scale works; mm matches this function's existing // ...depth in mm comment and DepthMilli naming. If the 65.535 m cap is a concern for long-range scenes, I'm happy to switch to centimeters (655 m range) or add a per-capture depth-scale setting instead — whichever you prefer.

[jonyMarino]

The depth milli is ultimately transmitted in 16 bits, so multiplying by 1000 in float16 would introduce a very small error; the biggest problem with float16 is overflow. There are different ways to tackle this, but for now, for ROS, what you can do is multiply the depth image you receive by 1000.

[jonyMarino]

The float32 should be converted to float16 or some other type that allows for a reduction in the amount of processing and copying in this step.

[andrewjong]

Ah okay! I see now, that makes sense. Implemented the change with some caveats:

from Claude:

Implemented in 37bdd2f: the depth wire now carries each pixel's render-target FFloat16 bit pattern verbatim (little-endian, new encoding label 16FC1, meters). The packing loop is now a pure bit copy — cheaper than both the old cast and the mm multiply — with no value conversion, no added quantization beyond the fp16 render target itself, no 65 m cap, and sky/no-hit arriving as +inf. Downstream decoders are updated in the same commit: the ROS2 C++ bridge decodes 16FC1 → standard ROS 32FC1 float meters (non-finite → NaN), the legacy Python rosbridge gets an equivalent converter, and the Python client's unpack_image returns a float16 array (the 16UC1 branches are kept for compatibility with older sims).

One small correction on the interim workaround suggested earlier ("for ROS, multiply the depth image you receive by 1000"): that couldn't work against the previous code — static_cast<uint16>(meters) is a value truncation, not a bit copy, so the wire carried whole meters and the sub-meter information was already destroyed before any later stage could scale it (a wall at 3.24 m arrived as exactly 3). With this commit the wire is the fp16 bits themselves, so downstream conversion is now lossless — verified end-to-end on our rig: the decoded 32FC1 stream shows fractional-millimeter depth structure and far geometry at ~4 km, both impossible under either previous packing.

Note this changes the depth wire encoding label from 16UC1 to 16FC1 intentionally, so older clients fail loudly (unknown encoding) rather than silently misreading fp16 bits as integers.

[jonyMarino]

Now I realize this was a real bug. Clients expecting 16UC1 should be fixed for 16FC1.
Can we separate the PR issue into the depth image problem and the clock issue?
Regarding the clock, it needs to be taken from the simulator (in #182 I saw it clearly), and the simulator is missing a topic (from Project AirSim, not ROS) that publishes the clock so the bridge only has to take it and forward it. We did this in another project, but we haven't implemented it here yet.
Thank you very much for your help!

[andrewjong]

Done — split as requested:

• This PR is now scoped to the depth fix + the fis_canceled_ client fix only (branch force-pushed; the two depth commits are squashed into one, and the client-side decoders — ROS2 C++ bridge, legacy Python rosbridge, Python client unpack_image — were already fixed for 16FC1 in the same commit).
• The clock/header-stamping commit was removed from this PR. Since /clock Topic Not Published and Unreal Engine 5.7 Build Fix #182 already addresses /clock publication and you have the sim-published clock topic planned, we won't PR that area for now — we briefly opened and then withdrew ROS2 bridge: stamp message headers/TF with sim time from each message's time_stamp #194 for it; the commit lives on our fork (castacks:fix/bridge-sim-time-stamps) if it's ever useful.

One technical note from Claude:

The header-stamping commit is complementary to a sim-published clock topic rather than an alternative: a native clock topic is the right replacement for the bridge's current GetSimTime polling of /clock, while per-message header stamps should come from each sample's own time_stamp regardless of how /clock is delivered. Worth keeping in mind when the native clock topic lands — happy to resubmit that piece rebased on it then.

[jonyMarino]

Thanks for your contribution! Please review the comments.