Implement manual deep copy for C3DData instead of relying on ezc3d's shallow copy constructor#277
Conversation
Cpp-Linter Report
|
Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com>
Replace #include <ezc3d/ezc3d.h> in c3ddata.h with a forward declaration of ezc3d::c3d, following the DICOM module pattern. Move DataTraits info() implementation to c3ddatatraits.cpp to keep the external library confined to .cpp files only. Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com>
…rame extraction Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com>
Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com>
9aa36b1 to
6e92b97
Compare
Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com> Agent-Logs-Url: https://github.com/inviwo/modules/sessions/802536ca-88dd-4e2a-939e-942170ddc40e
…py() Co-authored-by: petersteneteg <3638222+petersteneteg@users.noreply.github.com> Agent-Logs-Url: https://github.com/inviwo/modules/sessions/bf174c8c-18ac-496c-ba0d-bf3876b29763
| "Motion Tracking", // Category | ||
| CodeState::Experimental, // Code state | ||
| Tags::CPU | Tag{"C3D"}, // Tags | ||
| R"(<Explanation of how to use the processor.>)"_unindentHelp, |
There was a problem hiding this comment.
Cpp-linter Review
Used clang-tidy v19.1.1
Click here for the full clang-tidy patch
diff --git a/misc/c3d/src/processors/c3dpointalignment.cpp b/misc/c3d/src/processors/c3dpointalignment.cpp
index 675f3d3..2bb3669 100644
--- a/misc/c3d/src/processors/c3dpointalignment.cpp
+++ b/misc/c3d/src/processors/c3dpointalignment.cpp
@@ -62 +62 @@ glm::dmat4 toGLM(const Eigen::Matrix3d& R, const Eigen::Vector3d& t) {
-glm::dvec3 toGLM(const Eigen::Vector3d& v) { return glm::dvec3(v(0), v(1), v(2)); }
+glm::dvec3 toGLM(const Eigen::Vector3d& v) { return {v(0), v(1), v(2)}; }
Have any feedback or feature suggestions? Share it here.
|
|
||
| namespace inviwo { | ||
|
|
||
| class IVW_MODULE_C3D_API C3DModule : public InviwoModule { |
There was a problem hiding this comment.
clang-tidy diagnostic
misc/c3d/include/inviwo/c3d/c3dmodule.h:36:26: warning: [cppcoreguidelines-special-member-functions]
class 'C3DModule' defines a default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator
36 | class IVW_MODULE_C3D_API C3DModule : public InviwoModule {
| ^| return M; | ||
| } | ||
|
|
||
| glm::dvec3 toGLM(const Eigen::Vector3d& v) { return glm::dvec3(v(0), v(1), v(2)); } |
There was a problem hiding this comment.
clang-tidy diagnostics
- avoid repeating the return type from the declaration; use a braced initializer list instead [modernize-return-braced-init-list]
| glm::dvec3 toGLM(const Eigen::Vector3d& v) { return glm::dvec3(v(0), v(1), v(2)); } | |
| glm::dvec3 toGLM(const Eigen::Vector3d& v) { return {v(0), v(1), v(2)}; } |
| addProperties(includeResiduals_); | ||
| } | ||
|
|
||
| void C3DToDataFrame::process() { |
There was a problem hiding this comment.
clang-tidy diagnostic
misc/c3d/src/processors/c3dtodataframe.cpp:72:22: warning: [readability-function-cognitive-complexity]
function 'process' has cognitive complexity of 38 (threshold 25)
72 | void C3DToDataFrame::process() {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:88:9: note: +1, including nesting penalty of 0, nesting level increased to 1
88 | for (size_t f = 0; f < nbFrames; ++f) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:90:48: note: +2, including nesting penalty of 1, nesting level increased to 2
90 | timeValues[f] = (frameRate > 0.0f) ? static_cast<float>(f) / frameRate : 0.0f;
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:96:9: note: +1, including nesting penalty of 0, nesting level increased to 1
96 | for (size_t p = 0; p < nbPoints; ++p) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:103:13: note: +2, including nesting penalty of 1, nesting level increased to 2
103 | if (includeResiduals_) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:107:13: note: +2, including nesting penalty of 1, nesting level increased to 2
107 | for (size_t f = 0; f < nbFrames; ++f) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:112:17: note: +3, including nesting penalty of 2, nesting level increased to 3
112 | if (includeResiduals_) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:120:13: note: +2, including nesting penalty of 1, nesting level increased to 2
120 | if (includeResiduals_) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:137:9: note: +1, including nesting penalty of 0, nesting level increased to 1
137 | if (nbAnalogs > 0 && nbAnalogByFrame > 0) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:137:27: note: +1
137 | if (nbAnalogs > 0 && nbAnalogByFrame > 0) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:147:36: note: +2, including nesting penalty of 1, nesting level increased to 2
147 | (frameRate > 0.0f) ? frameRate * static_cast<float>(nbSubframes) : 0.0f;
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:149:13: note: +2, including nesting penalty of 1, nesting level increased to 2
149 | for (size_t f = 0; f < nbFrames; ++f) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:150:17: note: +3, including nesting penalty of 2, nesting level increased to 3
150 | for (size_t sf = 0; sf < nbSubframes; ++sf) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:155:45: note: +4, including nesting penalty of 3, nesting level increased to 4
155 | (analogRate > 0.0f) ? static_cast<float>(idx) / analogRate : 0.0f;
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:164:13: note: +2, including nesting penalty of 1, nesting level increased to 2
164 | for (size_t ch = 0; ch < nbAnalogs; ++ch) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:167:17: note: +3, including nesting penalty of 2, nesting level increased to 3
167 | for (size_t f = 0; f < nbFrames; ++f) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:169:21: note: +4, including nesting penalty of 3, nesting level increased to 4
169 | for (size_t sf = 0; sf < nbSubframes; ++sf) {
| ^
/home/runner/work/modules/modules/inviwo/misc/c3d/src/processors/c3dtodataframe.cpp:176:48: note: +3, including nesting penalty of 2, nesting level increased to 3
176 | (ch < channelNames.size()) ? channelNames[ch] : fmt::format("Channel_{}", ch);
| ^
ezc3d::c3dstoresHeader,Parameters, andDataasshared_ptrinternally, andFramesimilarly storesPoints,Analogs,Rotationsasshared_ptr. The copy constructor only copies the pointers, so the previousdeepCopy()viastd::make_shared<ezc3d::c3d>(*data_)produced a shallow copy where mutations to the copy would affect the original.Replaced with a manual recursive reconstruction:
c3d::point(name)andc3d::analog(name)to initialize the POINT/ANALOG configuration consistently in the new objectRotation::set()c3d::parameter(); POINT/ANALOG groups are skipped since they're already initialized by the registration step💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.