Implement OASIS file writer #674

c1ydehhx · 2026-02-04T12:12:49Z

In this PR, I added OASIS device. This device support writer only currently. This writer implement manhattan delta format by OASIS spec.

For example, if we expected +340 for N/S direction, it will be encoded with 0xA8 0x05, and -340 will be 0xA9 0x05.

I added some test to make sure it work properly, and I add some edge case to make sure it will not buggy when met edge case.

yungyuc

Points to address:

Write OASIS I/O code in C++. Name it modmesh::OasisDevice. Wrap it to Python for testing.
Manually verify the written file by loading it using a third-party viewer and paste the screenshot.

yungyuc · 2026-02-04T13:26:59Z

modmesh/plot/plane_layer.py

 # POSSIBILITY OF SUCH DAMAGE.


+class ManhattanDeltaCodec:


Write I/O code in C++. Python is too slow to be taken seriously for I/O.

Name it modmesh::OasisDevice for now.

yungyuc · 2026-02-04T13:27:53Z

tests/test_plane_layer.py

 from modmesh.plot import plane_layer


+class ManhattanDeltaCodecTC(unittest.TestCase):


Unit tests for the OASIS I/O should be in Python.

c1ydehhx · 2026-02-05T16:03:25Z

Hi @yungyuc,
So far away, I already migrate Manhatten 1-delta writer (Named it OasisWriter) from Python to C++, and use pybind11 to wrap it as a module.

Write OASIS I/O code in C++. Name it modmesh::OasisDevice. Wrap it to Python for testing.

Done.

Manually verify the written file by loading it using a third-party viewer and paste the screenshot.

I'm concern that it will have too much diff in this PR if we want to output OASIS file. Althought we have geometry record now, we don't have some necessary OASIS header like magic bytes, START and END.

I would like to confirm that this PR should support OASIS file output or we can just test Manhatten 1-delta work properly with unit tests?

yungyuc · 2026-02-06T04:46:02Z

Linter did not pass. Please fix code format and then request for review again.

yungyuc

Some very basic points to address:

Do not make an intrusive alias in global namespace and for STL things like std::vector<uint8)_t>.
Rule of 5 for OasisDevice.
Name as OasisDevice::write, not writer, which is not a verb.

yungyuc · 2026-02-06T05:18:33Z

cpp/modmesh/oasis/oasis_device.hpp

+#include <cstdint>
+#include <vector>
+
+using bytes = std::vector<uint8_t>;


Do not make an intrusive alias like this. It is horrible to maintain.

There is nothing wrong with using std::vector<uint8)_t>? Just spell out the type.

It is horrible to add an alias in the global name space. Unacceptable almost always.

yungyuc · 2026-02-06T05:20:08Z

cpp/modmesh/oasis/oasis_device.hpp

+    static void append_1_delta(bytes & segment, int value);
+
+public:
+    OasisDevice() = default;


yungyuc · 2026-02-06T05:20:42Z

cpp/modmesh/oasis/oasis_device.hpp

+
+public:
+    OasisDevice() = default;
+    static bytes writer(std::vector<int32_t> & values);


Name it write, not writer, which is not a verb.

yungyuc · 2026-02-06T05:21:57Z

@tigercosmos @KHLee529 please help review

tigercosmos · 2026-02-06T16:09:15Z

cpp/modmesh/oasis/pymod/wrap_oasis.cpp

+namespace python
+{
+
+using namespace modmesh::oasis; // NOLINT(google-build-using-namespace)


I recommend remove this. Namespace is important for reading the code.

Good catch. modmesh::oasis namespace is not necessary and should be removed.

And, never using namespace unless it is in a very limited scope and can resolve extreme pain.

tigercosmos · 2026-02-06T16:09:52Z

cpp/modmesh/oasis/oasis_device.cpp

+
+void OasisDevice::append_1_delta(std::vector<uint8_t> & segment, int value)
+{
+    int dir_bit = value < 0 ? 1 : 0;


Please add const for all the places if applicable.

tigercosmos · 2026-02-06T16:10:50Z

cpp/modmesh/oasis/oasis_device.hpp

+namespace oasis
+{
+
+class OasisDevice


Please add document comments for classes and functions.

tigercosmos · 2026-02-06T16:12:21Z

cpp/modmesh/oasis/oasis_device.cpp

+    }
+}
+
+std::vector<uint8_t> OasisDevice::write(std::vector<int32_t> & values)


const std::vector<int32_t> & values

tigercosmos · 2026-02-06T16:12:43Z

cpp/modmesh/oasis/oasis_device.cpp

+namespace oasis
+{
+
+void OasisDevice::append_1_delta(std::vector<uint8_t> & segment, int value)


This can be a local function in cpp only.

tigercosmos · 2026-02-06T16:13:35Z

cpp/modmesh/oasis/oasis_device.cpp

+    while (payload >= 1)
+    {
+        int first_bit = payload >= 128 ? 1 : 0;
+        segment.push_back((first_bit << 7) | (payload % 128));


Please add comments to explain the algorithm. Including source doucment is even better.

tigercosmos · 2026-02-06T16:14:22Z

modmesh/plot/plane_layer.py

 # POSSIBILITY OF SUCH DAMAGE.


+class ManhattanDeltaCodec:


Please remove it.

tigercosmos · 2026-02-06T16:15:07Z

tests/test_oasis_device.py

+        device = modmesh.OasisDevice()
+        seg = device.writer([+340, +200, -340, -40, +300, -120, -300])
+
+        self.assertEqual(seg, [0xD0, 0x0A, 0xA0, 0x06, 0xD2,


It will be nice if you can explain what you try to test in the comments.

Use code comments to explain.

yungyuc

Points to address:

Do not use namespace modmesh::oasis.
Add very concise code comments for the key points in unit tests.

yungyuc · 2026-02-07T00:44:35Z

cpp/modmesh/oasis/pymod/wrap_oasis.cpp

+namespace python
+{
+
+using namespace modmesh::oasis; // NOLINT(google-build-using-namespace)


Good catch. modmesh::oasis namespace is not necessary and should be removed.

And, never using namespace unless it is in a very limited scope and can resolve extreme pain.

yungyuc · 2026-02-07T00:45:04Z

cpp/modmesh/oasis/oasis_device.hpp

+namespace modmesh
+{
+
+namespace oasis


Do not use namespace modmesh::oasis.

@c1ydehhx You forgot to fix it.

yungyuc · 2026-02-07T01:25:35Z

tests/test_oasis_device.py

+        device = modmesh.OasisDevice()
+        seg = device.writer([+340, +200, -340, -40, +300, -120, -300])
+
+        self.assertEqual(seg, [0xD0, 0x0A, 0xA0, 0x06, 0xD2,


Use code comments to explain.

c1ydehhx · 2026-02-08T08:11:50Z

Hi @yungyuc and @tigercosmos.

Thanks for the review.

I complete OASIS device to store polygon and rectangle and write as OASIS format. The OASIS format will need magic-byte, START record, END record, CELLNAME record and CELL record to work. For each record, I add comment to describe its format and how to find the record spec in OASIS spec draft. I also add some comment to describe how unsigned and signed value work in OASIS. Please refer the comment in oasis_device.cpp. For the comment of tests. I leave a simple commant to let viewer refer the comment in oasis_device.cpp, which is more complete, and not leave too much words in test comment.

Also, the write() function in OASIS will return a list of bytes that represent the bytes in OASIS file. I write a simple script to check it works. The test script is not include in this PR. The script looks like:

import modmesh

device = modmesh.OasisDevice()

rec_poly = modmesh.OasisPolyRecord([
    [70, 720], [410, 720], [410, 920], [70, 920],
    [70, 880], [370, 880], [370, 760], [70, 760]])
rec_rect = modmesh.OasisRectRecord([70, 800], 180, 40)

device.add_poly_record(rec_poly)
device.add_rect_record(rec_rect)

rec_poly = modmesh.OasisPolyRecord([
    [70, 980], [410, 980], [410, 1180], [70, 1180],
    [70, 1140], [370, 1140], [370, 1020], [70, 1020]])
rec_rect = modmesh.OasisRectRecord([70, 1060], 180, 40)

device.add_poly_record(rec_poly)
device.add_rect_record(rec_rect)

with open("test.oas", "wb") as f:
    f.write(bytearray(device.write()))

It will generate test.oas file. The file in KLayout will looks like:

Please review it. Thanks.

yungyuc · 2026-02-08T09:39:48Z

cpp/modmesh/oasis/oasis_device.hpp

+class PolyRecord
+{
+private:
+    std::vector<std::pair<int, int>> vertexes;


All non-POD class member data need to prefix m_.

Put member data after member functions like other modmesh classes do. Follow convention unless you have a wonderful reason not to.

Use "vertices" as the plural form of "vertex". Do not use vertexes.

I dislike std::vector member data. But it can be here for a while. They should be changed to a pad soon.

yungyuc · 2026-02-08T09:43:12Z

cpp/modmesh/oasis/oasis_device.hpp

+namespace modmesh
+{
+
+namespace oasis


@c1ydehhx You forgot to fix it.

yungyuc · 2026-02-08T09:43:59Z

cpp/modmesh/oasis/oasis_device.hpp

+    void add_poly_record(const PolyRecord & record);
+    void add_rect_record(const RectRecord & record);
+
+    std::vector<uint8_t> write();


If returning a byte container, rename write to to_bytes.

yungyuc · 2026-02-08T09:45:12Z

cpp/modmesh/oasis/oasis_device.hpp

+ * \class PolyRecord
+ * \brief Convert Rect information to OASIS polygon record bytes.
+ */
+class PolyRecord


Rename PolyRecord to OasisRecordPoly.

yungyuc · 2026-02-08T09:45:26Z

cpp/modmesh/oasis/oasis_device.hpp

+ * \class RectRecord
+ * \brief Convert Rect information to OASIS rectangle record bytes.
+ */
+class RectRecord


Rename RectRecord to OasisRecordRect.

yungyuc · 2026-02-08T09:46:50Z

cpp/modmesh/oasis/oasis_device.hpp

+namespace oasis
+{
+
+static void append_signed_integer(std::vector<uint8_t> & segment, int value);


After removing the namespace modmesh::oasis, move these free functions to OasisDevice as static functions..

You forgot to move the free functions.

yungyuc · 2026-02-08T09:48:16Z

tests/test_oasis_device.py

+import modmesh
+
+
+class OasisRectRecordTC(unittest.TestCase):


Good one. Need to be renamed with the C++ class.

yungyuc · 2026-02-08T09:48:21Z

tests/test_oasis_device.py

+        self.assertEqual(record_bytes, list(map(ord, expected_record_bytes)))
+
+
+class OasisPolyRecordTC(unittest.TestCase):


Good one. Need to be renamed with the C++ class.

yungyuc · 2026-02-08T09:49:06Z

tests/test_oasis_device.py

+        self.assertEqual(record_bytes, list(map(ord, expected_record_bytes)))
+
+
+# For the format of OASIS, please refer OasisDevice comment in oasis_device.cpp


Make it clear with the namespace modmesh::OasisDevice.

yungyuc

Do not include any Python-related (pybind11 included) headers outside a pymod/ directory.

yungyuc · 2026-02-08T09:54:55Z

cpp/modmesh/oasis/oasis_device.hpp

+ */
+
+#include <modmesh/base.hpp>
+#include <pybind11/stl.h>


Do not import any Python-related (pybind11 included) headers outside a pymod/ directory.

c1ydehhx · 2026-02-08T10:56:19Z

Hi @yungyuc. Thanks for the review.

I already fix all issues in comment. I adjust the function name, class name, member name, and comment that mention the class name,. Moreover, I remove pybind header that include in wrong place and remove the oasis namespace.

For the comment that move free function to OasisDevice class, since all classes depends append_signed_integer, and append_unsigned_integer function, I can't move them into OasisDevice class. However, append_magic_bytes, append_start_record_bytes, append_cell_and_cell_name_record_byte and append_end_record_byte only used in OasisDevice, I move them to OasisDevice class.

Please review it. Thanks.

yungyuc · 2026-02-08T11:01:43Z

For the comment that move free function to OasisDevice class, since all classes depends append_signed_integer, and append_unsigned_integer function, I can't move them into OasisDevice class.

You can. Move OasisDevice to be before the declaration of OasisRecord*, and forward declare OasisRecord* before OasisDevice. See:

// Forward declaration (but do not add comment in your PR because it's obvious.
OasisRecordPoly;
OasisRecordRect;

class OasisDevice
{

public:

  // all static functions for OasisRecord*.

private:

  std::vector<OasisRecordPoly> //...;
  std::vector<OasisRecordRect> //...;

};

class OasisRecordPoly
{
  // content.s
};

class OasisRecordRect
{
  // content.s
};

yungyuc

Points to address:

Split the field OasisRecordRect::m_bottom_left to be m_lower and m_left.
Add end marker to all classes. You forgot some.
Missing rule of five.

yungyuc · 2026-02-08T11:02:26Z

cpp/modmesh/oasis/oasis_device.hpp

+    static void append_end_record_byte(std::vector<uint8_t> & segment);
+
+    template <typename T>
+    static void append_record_bytes(


It's too short to use two lines. Use a single line for the signature.

yungyuc · 2026-02-08T11:03:30Z

cpp/modmesh/oasis/oasis_device.hpp

+    std::vector<uint8_t> to_bytes() const;
+
+private:
+    std::pair<int, int> m_bottom_left;


Split the field m_bottom_left to be m_lower and m_left.

yungyuc · 2026-02-08T11:04:01Z

cpp/modmesh/oasis/oasis_device.hpp

+
+private:
+    std::pair<int, int> m_bottom_left;
+    int w;


m_ prefix. Please review carefully.

yungyuc · 2026-02-08T11:04:12Z

cpp/modmesh/oasis/oasis_device.hpp

+    std::pair<int, int> m_bottom_left;
+    int w;
+    int h;
+};


Class end marker.

yungyuc · 2026-02-08T11:05:01Z

cpp/modmesh/oasis/oasis_device.hpp

+public:
+    explicit OasisRecordPoly(std::vector<std::pair<int, int>> vertices)
+        : m_vertices(std::move(vertices)) {};
+    OasisRecordPoly(OasisRecordPoly const &) = default;


You forgot to delete default constructor.

yungyuc · 2026-02-08T11:05:30Z

cpp/modmesh/oasis/oasis_device.hpp

+        , h(h)
+    {
+    }
+    OasisRecordRect(OasisRecordRect const &) = default;


c1ydehhx · 2026-02-08T11:48:04Z

Hi @yungyuc. Thanks for the review.

I fix all issue mentioned in comment. I delete copy assignment on OasisDevice and record classes, add all end class marker and adjust signature to one-line, and split m_bottom_left to m_lower and m_left. Also, I check the member of non-POD class should have m_ prefix again. Sorry for the misunderstand of your instruction.

Please review it. Thanks.

yungyuc

You have previous points unaddressed:

You forgot to move free functions.
Missing rule of five.

c1ydehhx · 2026-02-08T15:27:04Z

Hi @yungyuc. Thanks for the review.

I just move the free function into OasisDevice class, and call these function in OasisRecord* class with static way. It looks like:

std::vector<uint8_t> OasisRecordRect::to_bytes(){
    // Implementation ...
    OasisDevice::append_unsigned_integer(segment, m_w);
    OasisDevice::append_unsigned_integer(segment, m_h);
    OasisDevice::append_signed_integer(segment, m_left);
    OasisDevice::append_signed_integer(segment, m_lower);
}

Also, I delete default constructor in OasisRecord*. I previous concern that delete default constructor will happen compilation error since the vector with OasisRecord* can not be construct.

    std::vector<OasisRecordPoly> m_polygon_records;
    std::vector<OasisRecordRect> m_rect_records;

After think a while, since I don't pre-allocate the space to let the vector using default constructor to create element, it still work properly.

I already fixed these issue. Thanks.

yungyuc

Also, I delete default constructor in OasisRecord*. I previous concern that delete default constructor will happen compilation error since the vector with OasisRecord* can not be construct.
    std::vector<OasisRecordPoly> m_polygon_records;
    std::vector<OasisRecordRect> m_rect_records;
After think a while, since I don't pre-allocate the space to let the vector using default constructor to create element, it still work properly.

I already fixed these issue. Thanks.

If you want to keep the default constructor, make it explicit like:

OasisRecordPoly() = default;

Otherwise it's deleted implicitly: https://stackoverflow.com/a/38995677.

The point is to make your intention explicit. You did not seem to know that not supplying the default constructor actually removed it. It will confuse you and maintainers in the future.

Review if you want to have the default constructor of OasisRecord* classes or not. If yes, change the code. If not, leave a comment and stay with the deletion.

Other points to address:

Move the static functions to the beginning of the class because they do not depend on other code in it.

yungyuc · 2026-02-08T23:21:38Z

cpp/modmesh/oasis/oasis_device.hpp

+    void add_poly_record(const OasisRecordPoly & record);
+    void add_rect_record(const OasisRecordRect & record);
+
+    static void append_signed_integer(std::vector<uint8_t> & segment, int value);


I usually move the static functions to the beginning of the class because they do not depend on other code in it.

It also makes sense to move the static member functions to be before other member functions, because the latter (or the code in the latter) may call the former.

My order is usually: (1) aliases, constants, and enum, (2) static member functions, (3) member functions, (4) member data. It's also the convention used in modmesh.

Please follow the convention.

yungyuc · 2026-02-08T23:36:54Z

cpp/modmesh/oasis/oasis_device.hpp

+    std::vector<uint8_t> to_bytes() const;
+
+private:
+    int m_left;


You may use 4 lines for the 4 fields or 1 line for them.

c1ydehhx · 2026-02-09T15:37:00Z

Hi @yungyuc. Thanks for the review.

I already fix the function order of class. If my realize correct, the member function may include constructor, so I put static function before constructor. I would like to check is it fit the convention. Thanks.

For the private static function, I'm not sure the convention that also need to move to the beginning of the class. If so, it will look like this:

class OasisDevice
{
public:
    static void append_signed_integer(std::vector<uint8_t> & segment, int value);
    static void append_unsigned_integer(std::vector<uint8_t> & segment, int value);

private:
    static void append_magic_bytes(std::vector<uint8_t> & segment);
    static void append_start_record_bytes(std::vector<uint8_t> & segment);
    static void append_cell_and_cell_name_record_byte(std::vector<uint8_t> & segment);
    static void append_end_record_byte(std::vector<uint8_t> & segment);

public:
    // Public member function.

private:
    // Private member function.
    // Private member.
} /* end class OasisDevice */

Please let me know that I just keep the current version or adjust to the version above.

Moreover, I condense rect member to 1 line.

Review if you want to have the default constructor of OasisRecord* classes or not. If yes, change the code. If not, leave a comment and stay with the deletion.

After think of the while, I think I'll keep default constructor deleted. Since the empty record of poly or rect is nonsense afaik, I'll keep the default constructor deleted. Thanks for the instruction about the behavior of constructor.

These changes already co-work with Codex to check the point you address is met.

yungyuc requested changes Feb 4, 2026

View reviewed changes

yungyuc assigned c1ydehhx Feb 4, 2026

yungyuc added the geometry Geometry entities and manipulation label Feb 4, 2026

yungyuc changed the title ~~Implement Manhattan Delta Codec writer~~ Implement OASIS file writer Feb 4, 2026

c1ydehhx force-pushed the manhattan-1-delta-writer branch from 8a2d55c to eb97718 Compare February 5, 2026 15:51

yungyuc requested changes Feb 6, 2026

View reviewed changes

yungyuc requested review from KHLee529 and tigercosmos February 6, 2026 05:21

c1ydehhx force-pushed the manhattan-1-delta-writer branch from 71b53ff to d3e62a3 Compare February 6, 2026 15:07

tigercosmos reviewed Feb 6, 2026

View reviewed changes

yungyuc requested changes Feb 7, 2026

View reviewed changes

c1ydehhx force-pushed the manhattan-1-delta-writer branch 4 times, most recently from 948efe1 to c7cec2a Compare February 8, 2026 07:55

yungyuc requested changes Feb 8, 2026

View reviewed changes

c1ydehhx force-pushed the manhattan-1-delta-writer branch 2 times, most recently from 47071d4 to 51e49fc Compare February 8, 2026 10:47

yungyuc requested changes Feb 8, 2026

View reviewed changes

c1ydehhx force-pushed the manhattan-1-delta-writer branch from 51e49fc to 17db390 Compare February 8, 2026 11:30

Add OASIS Device (Only writer)

fca59ca

c1ydehhx force-pushed the manhattan-1-delta-writer branch from 17db390 to fca59ca Compare February 8, 2026 11:35

yungyuc requested changes Feb 8, 2026

View reviewed changes

Move free function to OasisDevice

1f296c1

c1ydehhx force-pushed the manhattan-1-delta-writer branch from acd4ac1 to 150a8c9 Compare February 8, 2026 15:14

Delete default construct on OasisRecordPoly and OasisRecordRect

f2dffb9

c1ydehhx force-pushed the manhattan-1-delta-writer branch from 150a8c9 to f2dffb9 Compare February 8, 2026 15:21

yungyuc requested changes Feb 8, 2026

View reviewed changes

yungyuc reviewed Feb 8, 2026

View reviewed changes

Rreorder statics and condense rect members

af8f084

		from modmesh.plot import plane_layer


		class ManhattanDeltaCodecTC(unittest.TestCase):

		self.assertEqual(record_bytes, list(map(ord, expected_record_bytes)))


		class OasisPolyRecordTC(unittest.TestCase):

		self.assertEqual(record_bytes, list(map(ord, expected_record_bytes)))


		# For the format of OASIS, please refer OasisDevice comment in oasis_device.cpp

Implement OASIS file writer #674

Are you sure you want to change the base?

Implement OASIS file writer #674

Uh oh!

Conversation

c1ydehhx commented Feb 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1ydehhx commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yungyuc commented Feb 6, 2026

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yungyuc commented Feb 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tigercosmos Feb 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yungyuc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1ydehhx commented Feb 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1ydehhx commented Feb 4, 2026 •

edited

Loading

yungyuc left a comment •

edited

Loading

c1ydehhx commented Feb 5, 2026 •

edited

Loading

yungyuc left a comment •

edited

Loading

tigercosmos Feb 6, 2026 •

edited

Loading

yungyuc left a comment •

edited

Loading

c1ydehhx commented Feb 8, 2026 •

edited

Loading

yungyuc left a comment •

edited

Loading

yungyuc left a comment •

edited

Loading