refactor(common): extract shared PointXYZ / PCAFeature / plane-fit math (Part A of TBB work)#94
Merged
Merged
Conversation
cpp/patchwork/ and cpp/patchworkpp/ each carried their own copies of PointXYZ, PCAFeature, and the small geometry helpers (xy2theta, xy2radius, point_z_cmp). The plane-fit SVD itself lived in three slightly-different forms across cpp/patchwork/src/patchwork.cpp, cpp/patchworkpp/src/patchworkpp.cpp, and the upstream ~/git/patchwork. PR #90 (Fix 2) was a concrete bug caused by that drift. This commit factors them into a new tiny static library: cpp/common/ include/patchwork/types.h — PointXYZ, PCAFeature, PatchStatus include/patchwork/plane_fit.h — xy2theta, xy2radius, point_z_cmp, estimate_plane(seeds, out, th_dist) src/plane_fit.cpp — SVD-based plane fit (single canonical implementation) PCAFeature now carries the `principal_` (largest singular vector) field that exists in the original Patchwork repo; the classic Patchwork's PCAFeature was missing it. The current pipeline does not read `principal_` yet, but adding it costs ~12 bytes per patch and keeps the type signature aligned with the original. Dependents updated: - cpp/patchwork/include/patchwork/patchwork.h: delete duplicated PCAFeature/PatchStatus, include patchwork/types.h. - cpp/patchwork/src/patchwork.cpp: delete in-class xy2theta / xy2radius / estimate_plane, route call sites to the namespace-level helpers in patchwork/plane_fit.h. - cpp/patchworkpp/include/patchwork/patchworkpp.h: delete duplicated PointXYZ + xy2theta / xy2radius member decls, include patchwork/types.h. - cpp/patchworkpp/src/patchworkpp.cpp: delete its file-scope point_z_cmp and the PatchWorkpp::xy2theta / xy2radius definitions, include patchwork/plane_fit.h. `estimate_plane` is left untouched in patchworkpp.cpp (it still writes to instance-level pc_mean_/normal_/singular_values_) because refactoring it requires the thread-safety changes that will arrive with the TBB work in the next PR. CMake: new cpp/common/CMakeLists.txt builds ground_seg_common (static, PIC) linked against Eigen3::Eigen. Both classic and patchworkpp link to it. The patchworkpp build-tree export() set now also lists ground_seg_common to keep find_package(patchworkpp) usable from a build directory. Numerical equivalence verified on KITTI 00-10 full sweep (23,201 frames) for all four method × protocol combinations against the v1.3.1 baseline: every macro-average P / R / F1 cell within 0.0013 of the v1.3.1 number (two combinations byte-identical), well under the ±0.05 budget set in the refactor plan. Sets up the TBB parallelisation that will land in the follow-up PR.
This was referenced May 21, 2026
Merged
LimHyungTae
added a commit
that referenced
this pull request
May 21, 2026
Minor bump for the common-library refactor (#94) and the optional TBB parallelisation of classic Patchwork (#95). pypatchworkpp.patchwork gains 1.73x (120 -> 208 Hz on KITTI seq 00, i7-12700). pypatchworkpp.patchworkpp stays sequential after measurement — see #96 for the why. TBB is an optional build dependency: missing TBB falls back to a sequential loop with a CMake STATUS message; CI/wheel builds keep working without libtbb-dev installed. Bumps: - python/pyproject.toml 1.3.1 -> 1.4.0 - cpp/CMakeLists.txt 1.3.1 -> 1.4.0 CHANGELOG.md updated with the full v1.4.0 entry. See #94, #95, #96.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
First of two PRs (per the plan agreed in issue #89 discussion). Factors out the parts of the codebase that are independent of the Patchwork / Patchwork++ pipeline —
PointXYZ,PCAFeature,PatchStatus, the small geometry helpersxy2theta/xy2radius/point_z_cmp, and the plane-fit SVD itself — into a newcpp/common/static library:Before this PR the same code lived in three slightly-different copies (
cpp/patchwork/,cpp/patchworkpp/, and the upstream~/git/patchwork). Fix 2 in #90 was a real bug caused by that drift, so collapsing them is overdue.PCAFeaturenow carries theprincipal_field that exists in the upstream original — the classic Patchwork's PCAFeature was missing it. Costs ~12 bytes per patch and aligns the type signature with~/git/patchwork.estimate_planeis left untouched inpatchworkpp.cppfor this PR (it still writes to instance-levelpc_mean_/normal_/singular_values_). Refactoring it is required for the TBB work and lands in the follow-up PR.Numerical equivalence — passes by a wide margin
Full sweep KITTI 00-10 (23,201 frames), macro average, all four method × protocol combinations:
Two cells are byte-identical, the other two differ by ≤ 0.0013 P which is float-rounding noise. Budget per the plan was ±0.05 F1 macro and ±0.10 F1 per-seq; both well clear.
Diff stats
Test plan
pip install -v ./python/from a clean conda env builds.--method patchworkpp --eval_protocol patchworkpp).pre-commit runclean (clang-format applied auto-fixes once, then green).Follow-up
Part B (TBB parallelisation of Patchwork++'s
estimateGround) will land in a separate PR on top of this one. The signature change toestimate_plane(writing to anoutparameter instead of instance members) that's needed for thread-safety becomes a small follow-up patch then.Cross-refs: #89, #90, #91.