Allow reading/writing step files to string#1299
Allow reading/writing step files to string#1299anuraaga wants to merge 1 commit intotpaviot:masterfrom
Conversation
There was a problem hiding this comment.
PR Type: Enhancement
PR Summary: This pull request introduces functionality to read and write STEP files as strings, eliminating the need for temporary files on disk. This enhancement aligns with the interest expressed in issue #1096 and follows a pattern similar to that used in breptools. The implementation includes a comprehensive test case that demonstrates the process of writing a STEP file to a string, including setting up a document, creating a shape, assigning colors, and then reading the STEP data back from the string to verify the integrity of the shape and its properties.
Decision: Comment
📝 Type: 'Enhancement' - not supported yet.
- Sourcery currently only approves 'Typo fix' PRs.
✅ Issue addressed: this change correctly addresses the issue or implements the desired feature.
No details provided.
📝 Complexity: the changes are too large or complex for Sourcery to approve.
- Unsupported files: the diff contains files that Sourcery does not currently support during reviews.
General suggestions:
- Consider adding more detailed comments within the test case to explain each step of the process for future maintainability.
- Evaluate the performance implications of handling large STEP files as strings, especially in memory-constrained environments, and document any findings or recommendations.
- Explore the possibility of extending this functionality to other file formats supported by the library, providing a consistent interface for string-based file handling.
Thanks for using Sourcery. We offer it for free for open source projects and would be very grateful if you could help us grow. If you like it, would you consider sharing Sourcery on your favourite social media? ✨
| self.assertFalse(a_shape.IsNull()) | ||
|
|
||
| def test_read_write_step_string(self) -> None: | ||
| """Writes a step file as a string and reads it back""" |
There was a problem hiding this comment.
I thought that doing a round trip would be the simplest way to ensure both writing and reading have assertions applied to them. I noticed that test_write_step_file may not have any assertions being applied
|
Thank you for this interesting contribution. However, the thing is you still need to have the step file stored on your local filesystem, and I can't see the improvement compared to the standard with open("test.stp", "r", encoding="uft8") as step_file:
step_file_content = step_file.read()
step_reader.ReadFromString(step_file_content)This way, the |
|
Hi @tpaviot - I included the filename in Note It doesn't quite say what the purpose of the parameter is but seems to be required. One idea is to implement |
|
Good point ! If the filename is not necessary, let's just remove it (pass an empty string as filename), we don't need it at the python level (I have to check however why the filename is a mandatory parameter). FYI I dont modify swig files directly in the wrapper directory, I use to work with the pythonocc-generator:
|
|
Using an empty string seems to work, but there maybe some side effects I can not see so far. |
|
I modified the example https://github.com/tpaviot/pythonocc-demos/blob/master/examples/core_load_step_ap203_ocaf.py so that it can taka a string, that seems to work the same way as using a file. Maybe there would be a more explicit solution than just passing an empty string as the stream name. Or leave this parameter with a default value, for instance: def ReadFromString(the_string, the_stream_name="pythonocc_stream_name"):@anuraaga your opinion? |
|
Reverse engineering of the
It simply calls the
The stream name parameter is used at line https://github.com/Open-Cascade-SAS/OCCT/blob/master/src/STEPControl/STEPControl_Reader.cxx#L201 : WS()->SetLoadedFile(theName);The The stream is read by calling the method It just returns
void SetLoadedFile (const Standard_CString theFileName)
{ theloaded = theFileName; }
//! Returns the filename used to load current model
//! empty if unknown
Standard_CString LoadedFile() const
{ return theloaded.ToCString(); }
Conclusion I guess we can safely use an hardcoded string for the stream name parameter. We can wrap it as an optional parameter in python, if ever someone needs to set it to a different value. |
|
Thanks for confirming! In that case I lean towards not exposing a parameter, and if for some reason someone asks for it it could be added with a default value to be backwards compatible (I think). I somehow expect for no one to ever ask for it.
Sorry I hadn't noticed this repo. Should I move this change to it? And also
|
|
I've been working on a more robust solution : wrap all std::istream and std::ostream parameters in occt to python strings. This leads to a simpler generator, and a python wrapper more aligned with occt method names and signature. It maybe less pythonic, but easier to go from the official documentation to python. PR is on the way |
|
Thanks! That looks very useful |
|
I am trying to load a step file from a stream instead directly from a file. However, the following function fails:
I am passing this file_content: It is in utf-8 format. I assume that the file_content argument is incorrect (but str is expected). Thank you! |
In #1096 there is great interest in being able to marshal step files without temporary files on disk. While
*Streammethods are provided, they are awkward to use with Python files, and I think in some places in the library this is acknowledged and methods that read and write strings are provided. Strings do require buffering so may not be ideal compared to a stream in all cases, but in many cases it actually already is buffered, and it is still a significant improvement over temporary files for many of the cases listed in the issue, it would be great to be able to use this without files.I mostly copied the pattern of breptools
https://github.com/tpaviot/pythonocc-core/blob/master/src/SWIG_files/wrapper/BRepTools.i#L855
If there's a reason to avoid this, no worries in closing this as it's out of the blue. But since the change wasn't big I figured I'd give it a shot.