-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding in memory support for xyz files #5866
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. |
Please retrigger workflows when possible, thank you 😅 |
@yxlao I've made some updates, thanks |
@ssheorey Please re-run workflows, I've pushed a change that hopefully fixes windows build, thanks |
Any pointers as to why I'd be getting these errors on the windows pipelines are welcome. Short of trying to setup a development environment on windows myself (mainly use osx and linux), I'm not sure 100% how this is working on osx/linux but not on Windows unless I missed something obvious during building. I haven't tried to touch the code under
Edit: Fixed in 4284a20 |
@ssheorey workflow run please :), I tested on windows seems to build now 🙌 (also rebased w/ latest master for consistency) |
a2850ea
to
4284a20
Compare
Hi @yxlao can you take another look? Thanks. |
7f17aaf
to
89fa0eb
Compare
89fa0eb
to
8a76a28
Compare
Hi @yxlao can you take another look at this PR? Thanks. |
Added Python tests
Using {} initializer for WritePointCloudOption on tests/t/io/PointCloudIO.cpp
Thanks for your contributions! Is this feature accessible now? It would be really helpful for me, but I cannot find any API descriptions in the documents. |
I could help review this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 11 files reviewed, 9 unresolved discussions (waiting on @samypr100, @ssheorey, and @yxlao)
cpp/open3d/io/PointCloudIO.h
line 95 at r4 (raw file):
// Attention: when you update the defaults, update the docstrings in // pybind/io/class_io.cpp std::string format = "auto",
ES.23: Prefer the {}
-initializer syntax
cpp/open3d/io/file_format/FileXYZ.cpp
line 67 at r5 (raw file):
reporter.SetTotal(static_cast<int64_t>(length)); std::string content(reinterpret_cast<const char *>(buffer), length);
This create another copy of the buffer as std::string deep copy the input buffer. Also in the class_io.cpp we use std::memcpy.
Is there a way to process the buffer directly using char*? Maybe using std::istrstream.
@charangodwin re istratream, see #5866 (comment) about some history on how changing may affect benchmarks significantly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me with just a couple of minor suggestions.
Thanks @samypr100 for this very useful contribution and sorry for the delay in getting this merged - we'll try to merge faster in the future.... Minor comment about other formats (xyzn, xyzi, xyzrgb) - these are better supported in the more generic Tensor based datatypes (open3d::t::io namespace). Functions for reading from a file are already available there. |
Type
Motivation and Context
The ability to read/write formats in memory is a typical ask due to a variety of use cases. Open3D has some in memory options for file formats such as PNG/JPG (e.g. ReadPNGFromMemory) so it's useful if other file formats had this kind of support.
Checklist:
python util/check_style.py --apply
to apply Open3D code styleto my code.
updated accordingly.
results (e.g. screenshots or numbers) here.
Description
The changes in this PR doesn't fully close #1560 since I believe there's other formats to still support such as
xyzn
,xyzrgb,
pts
,pcd
, and maybe others.This PR only adds full "in memory" support for
xyz
file format in order to keep the PR small, simple, and reviewable. It's also early so if somethings needs to change with the approach it's easier rather than trying to retro-fit to all formats.Since we're reading contents from memory,
"auto"
would not quite work as there's no magic headers for all these files, so some changes were made so thatformat
is specified for both Reading/Writing Point Clouds. I'm using a prefix ofmem::{format}
for the new in memory formats.It simplifies the Python API when temporary files are used to achieve similar behavior like shown below.
The issue asked to mimic something like the file-like interface in
BytesIO()
, but Pybind currently doesn't "officially" offer that functionality despite being discussed in pybind/pybind11#1477.There's a solution to it from that issue, but that would mean bundling a custom pybind extension into the build process, which we could explore. The benefit of it is that we'd be able to retain a file-like interface rather than sending a buffer around, although I'm not sure if it will work with
FILE
though and some of the other more advanced File System IO that's done through the codebase.Laslty, with the current approach it's possible there'll be a decent amount of code duplication (similar to FileJPG.cpp, and FilePNG.cpp). The existing code seems tightly coupled with the File IO, so I'm open to suggestions/recommendations/alternatives.
This change is