Skip to content

Feature/track saving - #43

Open
MultyXu wants to merge 16 commits into
dcistfrom
feature/track_saving
Open

Feature/track saving#43
MultyXu wants to merge 16 commits into
dcistfrom
feature/track_saving

Conversation

@MultyXu

@MultyXu MultyXu commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

PR along with #41, still following #30 to clean up the code. PR points to #41 to better visualize only the track-saving changes.

Will rebase on top of #41 after the change detection code is merged.

@MultyXu
MultyXu marked this pull request as ready for review September 3, 2026 14:12
@MultyXu MultyXu mentioned this pull request Sep 3, 2026

@nathanhhughes nathanhhughes left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs a little bit more work before I'm happy with it, but it seems like this shouldn't break anything and the new debugging utilities are mostly useful. If the boxmot changes require this PR specifically, it's probably okay for this to go in and then I can go clean it up when I get a chance, otherwise we can coordinate who cleans this up and when it actually goes in.
Note: I looked at most of the PR, but kept comments minimal for now (they're representative of my general feedback though)

Comment on lines +139 to +141
* Extracted from computeIoUPixels so debugging/visualization tools (e.g. test_track_association)
* can obtain the same reprojected pixel set the tracker's own pixel-IoU comparison uses, without
* duplicating the formula.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not useful as a comment

Comment on lines +144 to +145
* @return The set of image pixels the points project onto (points that fail to project, e.g.
* behind the camera, are omitted).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop the parenthetical note

* @return The set of image pixels the points project onto (points that fail to project, e.g.
* behind the camera, are omitted).
*/
std::set<Pixel> reprojectPoints(const FrameData& data, const Points& points) const;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty sure this can be a static method

Comment on lines +38 to +58
/**
* Step 4 of the tracker-debugging pipeline (see tracker_debug.md): replays an ENTIRE saved run
* through a fresh MaxIoUTracker, frame by frame in chronological order, with the real multi-object
* competition production has (every cluster in every frame, not the single-pair filtering
* test_track_association.cpp does for Step 2). Lets different tracker configs (min_semantic_iou,
* track_by, etc.) be evaluated end-to-end against the same recorded data.
*
* Also simulates active-window eviction the same way production does: after each frame's
* association, every track's is_active is recomputed via the real hydra::VolumetricWindow
* (SpatialWindowChecker/TemporalWindowChecker) inBounds() check, exactly mirroring
* ActiveWindow::updateTrackingStatus (active_window.cpp). Evicted tracks are erased from the live
* list (mirroring ActiveWindow::extractInactiveObjects) and their final state archived to disk --
* once evicted, a track can never be re-associated again, matching production.
*
* KNOWN APPROXIMATION: pose.txt records getSensorPose() = real_world_T_body * real_body_T_sensor
* from the original run. FrameData::load's reconstructed camera uses identity extrinsics, so
* input.world_T_body is set to that recorded pose directly -- correct for the tracker's own
* reprojection math, but when reused here as "the robot body pose" for the eviction-radius check,
* it is off by the real camera's mounting offset (typically tens of cm). Negligible at the
* deployed 14m threshold; not solved (would require saving the real extrinsics).
*

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can probably drop this

Comment on lines +338 to +379
void saveCameraIntrinsics(const hydra::Camera& camera, const std::string& run_dir) {
const auto& cfg = camera.getConfig();
std::ofstream stream(run_dir + "/camera_intrinsics.json");
if (!stream.is_open()) {
LOG(ERROR) << "[FrameData] Failed to open camera_intrinsics.json for writing in " << run_dir;
return;
}
const json j{{"fx", cfg.fx},
{"fy", cfg.fy},
{"cx", cfg.cx},
{"cy", cfg.cy},
{"width", cfg.width},
{"height", cfg.height}};
stream << j.dump(2);
}

std::shared_ptr<hydra::Camera> loadCameraIntrinsics(const std::string& run_dir) {
std::ifstream file(run_dir + "/camera_intrinsics.json");
if (!file.is_open()) {
LOG(ERROR) << "[FrameData] Missing camera_intrinsics.json in " << run_dir;
return nullptr;
}
json j;
file >> j;

hydra::Camera::Config config;
// Sensor::Config requires min_range > 0 and max_range > min_range; the sink does not save the
// original sensor's range limits, so use permissive defaults wide enough for indoor/outdoor use.
config.min_range = 0.01;
config.max_range = 100.0;
config.fx = j.at("fx").get<float>();
config.fy = j.at("fy").get<float>();
config.cx = j.at("cx").get<float>();
config.cy = j.at("cy").get<float>();
config.width = j.at("width").get<int>();
config.height = j.at("height").get<int>();
// Identity extrinsics: the saved pose.txt already IS the sensor pose in world frame, so using
// it directly as world_T_body with identity body_T_sensor reproduces getSensorPose() exactly.
config.extrinsics = hydra::IdentitySensorExtrinsics::Config{};

return std::make_shared<hydra::Camera>(config, "reconstructed_camera");
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should be able to load and save the sensor information via config::toYaml and config::fromYaml (though I need to fix something in Hydra for whether or not the sensor is a camera or not before you can easily load the full sensor after the fact). This would also fix the issue with the extrinsics not being saved

Comment on lines +63 to +65
cv::Mat bgr_image;
cv::cvtColor(data.input.color_image, bgr_image, cv::COLOR_RGB2BGR);
cv::imwrite(dir + "/color.png", bgr_image);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't bother swapping RGB and BGR order on both save and load

Comment on lines +69 to +79
// 16-bit PNG scaled to mm for easy viewing.
cv::Mat depth_mm;
data.input.depth_image.convertTo(depth_mm, CV_16UC1, 1000.0);
cv::imwrite(dir + "/depth.png", depth_mm);

// Raw float32 binary for exact values.
std::ofstream depth_stream(dir + "/depth.bin", std::ios::binary);
if (depth_stream.is_open()) {
depth_stream.write(reinterpret_cast<const char*>(data.input.depth_image.data),
data.input.depth_image.total() * data.input.depth_image.elemSize());
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would be better as cv::imwrite(dir + "/depth.tiff", data.input.depth_image);. Most image viewers should support it and it doesn't lose information

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants