Conversation
|
You are modifying libf3d public API! |
| .def(py::init<>()) | ||
| .def(py::init<const std::filesystem::path&>()) | ||
| .def(py::init<unsigned int, unsigned int, unsigned int, f3d::image::ChannelType>()) | ||
| .def(py::init([](py::bytes buffer, size_t size){ |
There was a problem hiding this comment.
| .def(py::init([](py::bytes buffer, size_t size){ | |
| .def(py::init([](py::bytes buffer){ |
Unlike in C which uses a raw pointer, the length can and should be retrieved from the Python buffer object, see setImageBytes above.
I believe this also applies to Java and WebAssembly
There was a problem hiding this comment.
Unlike in C which uses a raw pointer, the length can and should be retrieved from the Python buffer object, see
setImageBytesabove.I believe this also applies to Java and WebAssembly
Would there ever be a situation where the user wants to read only part of the buffer?
There was a problem hiding this comment.
Would there ever be a situation where the user wants to read only part of the buffer?
I don't think so, in python I'd slice the buffer with buffer[:n] to keep only the first n bytes. IMO, if we were concerned about allowing to read only a part of a bigger buffer (for performance/memory reasons) we'd need to have something like f(buffer, start, end) or f(buffer, start, len).
From what I can tell this whole extra size/end parameter thing is only because the technical limitation in C were pointers represent the start of the memory region and nothing else. @Meakk am I missing something?
There was a problem hiding this comment.
That's right, let's drop the size argument, otherwise it will look odd from Python
There was a problem hiding this comment.
this should not be commited, also do not configure in source to avoid this.
| #include <sstream> | ||
| #include <string> | ||
| #include <unordered_map> | ||
| #include <iostream> //debug |
| while (reader != nullptr) | ||
| { | ||
| int score = reader->CanReadFile(stream); | ||
| if (score > bestScore) | ||
| { | ||
| bestReader = reader; | ||
| bestScore = score; | ||
| } | ||
|
|
||
| reader = collection->GetNextItem(); | ||
| } |
There was a problem hiding this comment.
there is no API to do this for you in VTK ?
There was a problem hiding this comment.
there is no API to do this for you in VTK ?
I'm looking at vtkImageReader2 docs, and I don't see an API that does this
I'm also looking at the relevant commits in vtk and there is CanReadFile(stream) support for the different readers but not a function that consolidates and finds best reader based on the stream buffer
There was a problem hiding this comment.
I think I forgot to add it, ill take a look, but lets of course keep it here for now.
| image& operator=(const image& img) noexcept; | ||
| image(image&& img) noexcept; | ||
| image& operator=(image&& img) noexcept; | ||
| image(std::byte* byte, std::size_t size); |
There was a problem hiding this comment.
document separately and add doc about the behavior, espacially the exceptions
mwestphal
left a comment
mwestphal
left a comment
There was a problem hiding this comment.
need some changes, lets focus on the C++ api first.
4 participants
Describe your changes
Add image(buffer, size) API
Requires vtk version that contains CanReadFile(stream) API (> 9.6)
https://gitlab.kitware.com/vtk/vtk/-/commit/675e97fa64a9563f6ec6f1fbf06aba84b2920ea8
Having a bit of trouble finding the right type conversions for Javascript bindings, but a review of what I have so far & feedback would be much appreciated!
Issue ticket number and link if any
Checklist for finalizing the PR
.github/workflows/versions.json, I have updateddocker_timestampContinuous integration
Please write a comment to run CI, eg:
\ci fast.See here for more info.