Simplify data_ and data_interface_ in KDTreeFlann. #6734
Conversation
|
Thanks for submitting this pull request! The maintainers of this repository would appreciate if you could update the CHANGELOG.md based on your changes. |
PieroV
force-pushed
the
flann-member-data
branch
from
3a9c694
to
46216d8
Compare
Seriously though, the CI could at least give a patch to apply to make the formatting pass 😅. |
KDTreeFlann::SetRawData had some confusion around data, data_, and data_interface_. As a result, the data from the parameter was copied to the member std::vector, but the interface (which takes an Eigen::Map) used the data passed as an argument to the method. As a result, the searches risked to be run on a dangling pointer, instead of the intended internal storage. However, rather than adjusting the pointer for Eigen::Map, I preferred simplifying the code, and copy the argument to an Eigen::MatrixXd.
PieroV
force-pushed
the
flann-member-data
branch
from
46216d8
to
3ed4e34
Compare
Sorry about that - the CI script can actually fix the formatting for you:
A great quality of life improvement would be to make an automatic bot commit this in the CI. clang-format-10 should be (perpetually?) available through |
|
No worries, eventually I managed to format 😄.
Thanks, I didn't know this! |
Type
Motivation and Context
KDTreeFlann::SetRawDatahad some confusion arounddata,data_, anddata_interface_.As a result, the data from the parameter was copied to the member
std::vector, but the interface (which uses anEigen::Map) used the data passed as an argument to the method.As a result, the searches risked to be run on a dangling pointer, instead of the intended internal storage.
This produced unexpected/wrong results sometimes.
Checklist:
python util/check_style.py --applyto apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
The most surgical change would make change the existing
Eigen::Mapto refer the internal storage.However, I noticed that the 4 members (
data_,data_interface_,dimension_,dataset_size_) were all to be kept in sync.So, I preferred simplifying the code, copy the argument to an
Eigen::MatrixXdand remove everything else (I replaced the dataset size and dimension with the rows and cols of the new matrix, and the matrix itself is the owner of the storage).I noticed the data was
protected, rather thanprivate, but the build didn't break.I will update the fix if subclasses are borken.