Conversation
modmesh/plot/plane_layer.py
Outdated
| # POSSIBILITY OF SUCH DAMAGE. | ||
|
|
||
|
|
||
| class ManhattanDeltaCodec: |
There was a problem hiding this comment.
Write I/O code in C++. Python is too slow to be taken seriously for I/O.
Name it modmesh::OasisDevice for now.
tests/test_plane_layer.py
Outdated
| from modmesh.plot import plane_layer | ||
|
|
||
|
|
||
| class ManhattanDeltaCodecTC(unittest.TestCase): |
There was a problem hiding this comment.
Unit tests for the OASIS I/O should be in Python.
8a2d55c to
eb97718
Compare
|
Hi @yungyuc,
Done.
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? |
|
Linter did not pass. Please fix code format and then request for review again. |
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| #include <cstdint> | ||
| #include <vector> | ||
|
|
||
| using bytes = std::vector<uint8_t>; |
There was a problem hiding this comment.
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.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| static void append_1_delta(bytes & segment, int value); | ||
|
|
||
| public: | ||
| OasisDevice() = default; |
cpp/modmesh/oasis/oasis_device.hpp
Outdated
|
|
||
| public: | ||
| OasisDevice() = default; | ||
| static bytes writer(std::vector<int32_t> & values); |
There was a problem hiding this comment.
Name it write, not writer, which is not a verb.
|
@tigercosmos @KHLee529 please help review |
71b53ff to
d3e62a3
Compare
| namespace python | ||
| { | ||
|
|
||
| using namespace modmesh::oasis; // NOLINT(google-build-using-namespace) |
There was a problem hiding this comment.
I recommend remove this. Namespace is important for reading the code.
There was a problem hiding this comment.
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.
cpp/modmesh/oasis/oasis_device.cpp
Outdated
|
|
||
| void OasisDevice::append_1_delta(std::vector<uint8_t> & segment, int value) | ||
| { | ||
| int dir_bit = value < 0 ? 1 : 0; |
There was a problem hiding this comment.
Please add const for all the places if applicable.
| namespace oasis | ||
| { | ||
|
|
||
| class OasisDevice |
There was a problem hiding this comment.
Please add document comments for classes and functions.
cpp/modmesh/oasis/oasis_device.cpp
Outdated
| } | ||
| } | ||
|
|
||
| std::vector<uint8_t> OasisDevice::write(std::vector<int32_t> & values) |
There was a problem hiding this comment.
const std::vector<int32_t> & values
cpp/modmesh/oasis/oasis_device.cpp
Outdated
| namespace oasis | ||
| { | ||
|
|
||
| void OasisDevice::append_1_delta(std::vector<uint8_t> & segment, int value) |
There was a problem hiding this comment.
This can be a local function in cpp only.
| while (payload >= 1) | ||
| { | ||
| int first_bit = payload >= 128 ? 1 : 0; | ||
| segment.push_back((first_bit << 7) | (payload % 128)); |
There was a problem hiding this comment.
Please add comments to explain the algorithm. Including source doucment is even better.
modmesh/plot/plane_layer.py
Outdated
| # POSSIBILITY OF SUCH DAMAGE. | ||
|
|
||
|
|
||
| class ManhattanDeltaCodec: |
tests/test_oasis_device.py
Outdated
| device = modmesh.OasisDevice() | ||
| seg = device.writer([+340, +200, -340, -40, +300, -120, -300]) | ||
|
|
||
| self.assertEqual(seg, [0xD0, 0x0A, 0xA0, 0x06, 0xD2, |
There was a problem hiding this comment.
It will be nice if you can explain what you try to test in the comments.
| namespace python | ||
| { | ||
|
|
||
| using namespace modmesh::oasis; // NOLINT(google-build-using-namespace) |
There was a problem hiding this comment.
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.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| namespace modmesh | ||
| { | ||
|
|
||
| namespace oasis |
There was a problem hiding this comment.
Do not use namespace modmesh::oasis.
tests/test_oasis_device.py
Outdated
| device = modmesh.OasisDevice() | ||
| seg = device.writer([+340, +200, -340, -40, +300, -120, -300]) | ||
|
|
||
| self.assertEqual(seg, [0xD0, 0x0A, 0xA0, 0x06, 0xD2, |
948efe1 to
c7cec2a
Compare
|
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 Also, the 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
Please review it. Thanks. |
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| class PolyRecord | ||
| { | ||
| private: | ||
| std::vector<std::pair<int, int>> vertexes; |
There was a problem hiding this comment.
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.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| namespace modmesh | ||
| { | ||
|
|
||
| namespace oasis |
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| void add_poly_record(const PolyRecord & record); | ||
| void add_rect_record(const RectRecord & record); | ||
|
|
||
| std::vector<uint8_t> write(); |
There was a problem hiding this comment.
If returning a byte container, rename write to to_bytes.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| * \class PolyRecord | ||
| * \brief Convert Rect information to OASIS polygon record bytes. | ||
| */ | ||
| class PolyRecord |
There was a problem hiding this comment.
Rename PolyRecord to OasisRecordPoly.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| * \class RectRecord | ||
| * \brief Convert Rect information to OASIS rectangle record bytes. | ||
| */ | ||
| class RectRecord |
There was a problem hiding this comment.
Rename RectRecord to OasisRecordRect.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| namespace oasis | ||
| { | ||
|
|
||
| static void append_signed_integer(std::vector<uint8_t> & segment, int value); |
There was a problem hiding this comment.
After removing the namespace modmesh::oasis, move these free functions to OasisDevice as static functions..
There was a problem hiding this comment.
You forgot to move the free functions.
tests/test_oasis_device.py
Outdated
| import modmesh | ||
|
|
||
|
|
||
| class OasisRectRecordTC(unittest.TestCase): |
There was a problem hiding this comment.
Good one. Need to be renamed with the C++ class.
tests/test_oasis_device.py
Outdated
| self.assertEqual(record_bytes, list(map(ord, expected_record_bytes))) | ||
|
|
||
|
|
||
| class OasisPolyRecordTC(unittest.TestCase): |
There was a problem hiding this comment.
Good one. Need to be renamed with the C++ class.
tests/test_oasis_device.py
Outdated
| self.assertEqual(record_bytes, list(map(ord, expected_record_bytes))) | ||
|
|
||
|
|
||
| # For the format of OASIS, please refer OasisDevice comment in oasis_device.cpp |
There was a problem hiding this comment.
Make it clear with the namespace modmesh::OasisDevice.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| */ | ||
|
|
||
| #include <modmesh/base.hpp> | ||
| #include <pybind11/stl.h> |
There was a problem hiding this comment.
Do not import any Python-related (pybind11 included) headers outside a pymod/ directory.
47071d4 to
51e49fc
Compare
|
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 For the comment that move free function to Please review it. Thanks. |
You can. Move // 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
}; |
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| static void append_end_record_byte(std::vector<uint8_t> & segment); | ||
|
|
||
| template <typename T> | ||
| static void append_record_bytes( |
There was a problem hiding this comment.
It's too short to use two lines. Use a single line for the signature.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| std::vector<uint8_t> to_bytes() const; | ||
|
|
||
| private: | ||
| std::pair<int, int> m_bottom_left; |
There was a problem hiding this comment.
Split the field m_bottom_left to be m_lower and m_left.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
|
|
||
| private: | ||
| std::pair<int, int> m_bottom_left; | ||
| int w; |
There was a problem hiding this comment.
m_ prefix. Please review carefully.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| std::pair<int, int> m_bottom_left; | ||
| int w; | ||
| int h; | ||
| }; |
| public: | ||
| explicit OasisRecordPoly(std::vector<std::pair<int, int>> vertices) | ||
| : m_vertices(std::move(vertices)) {}; | ||
| OasisRecordPoly(OasisRecordPoly const &) = default; |
There was a problem hiding this comment.
You forgot to delete default constructor.
| , h(h) | ||
| { | ||
| } | ||
| OasisRecordRect(OasisRecordRect const &) = default; |
17db390 to
fca59ca
Compare
|
Hi @yungyuc. Thanks for the review. I fix all issue mentioned in comment. I delete copy assignment on Please review it. Thanks. |
150a8c9 to
f2dffb9
Compare
|
Hi @yungyuc. Thanks for the review. I just move the free function into 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 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. |
There was a problem hiding this comment.
Also, I delete default constructor in
OasisRecord*. I previous concern that delete default constructor will happen compilation error since the vector withOasisRecord*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.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| 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); |
There was a problem hiding this comment.
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.
cpp/modmesh/oasis/oasis_device.hpp
Outdated
| std::vector<uint8_t> to_bytes() const; | ||
|
|
||
| private: | ||
| int m_left; |
There was a problem hiding this comment.
You may use 4 lines for the 4 fields or 1 line for them.
|
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.
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
left a comment
There was a problem hiding this comment.
LGTM
Please let me know that I just keep the current version or adjust to the version above.
For private static functions, either works.
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.
Not necessarily. We usually reserve in containers. But maybe reserving can use placement new. It would be interesting to check what the standard says about it.
yungyuc
left a comment
There was a problem hiding this comment.
Please update commit logs and the PR leading comment with a concise summary of what you do.
You do not have to squash although you may. Request for the next review once it's done.
ef19a45 to
3c871db
Compare
|
Hi @yungyuc. I alreday update PR description and squash the commit and add properly message. Please merge it after CI passed. Thanks. |
You already squashed. But the commit log does not have a clear summary. This is what I see:
Please try again. |
a7b65d9 to
b60839a
Compare
|
Hi @yungyuc. I already rewrite the commit message that try to let it more clear. Please review it. Thanks. |
b60839a to
05251c0
Compare
You wrote a short commit log but it's not clear. Try this:
Please supplement missing information and rewrite to make it more clear and concise. |
cpp/modmesh/oasis/oasis_device.cpp
Outdated
| for (char c : magic_byte_plain_text) | ||
| { | ||
| segment.push_back(c); | ||
| } |
There was a problem hiding this comment.
why not copy the whole buffer directly?
There was a problem hiding this comment.
same for all other similar operations.
There was a problem hiding this comment.
I think it will be better if I copy whole buffer directly. Thanks for the suggestion.
There was a problem hiding this comment.
Hi @tigercosmos. I changes few byte append operation with for-loop with std::vector insert operation instead, which makes the code clear. For the other byte append operation with std::vector::push_back function, I would like to append bytes sepreately and with comments to clarify how the OASIS format is organized.
|
@c1ydehhx Are you ready for updating the commit log? |
05251c0 to
3ba18a2
Compare
|
Hi @yungyuc. Thanks for the review. I've been updated the commit message with short description summarizing what this PR does. |
3ba18a2 to
d14125e
Compare
… `modmesh::OasisRecordRect` to output OASIS-format bytes - `OasisRecordPoly` class accepts coordinate pairs and converts them into OASIS Polygon format bytes. - `OasisRecordRect` class accepts a lower-left coordinate pair, width, height and converts them into OASIS Rectangle format bytes. - `OasisWriter` class accepts `OasisRecordPoly` and `OasisRecordRect` objects and integrates these records with the minimal required headers (e.g., magic-bytes, START, END, CELL, and CELLNAME records). The classes above are wrapped as Python classes via pybind11. Unit tests are added for the following classes: - `OasisRecordPoly` and `OasisRecordRect`: verify that the produced OASIS-format bytes are correct. - `OasisWriter`: verify the output bytes contain the required headers with integrated polygon and rectangle records.
d14125e to
7e3fa42
Compare
|
Hi @yungyuc. Thanks for the review. I've updated the lead comment and commit message. Please review it. Thanks. |

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 0xD0 0x0A0xA8 0x05, and -340 will be 0xD2 0x0A0xA9 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.
Updated (2026/02/19):
After the discussion with @yungyuc, I added 3 classes
modmesh::OasisWriter,modmesh::OasisRecordRect,modmesh::OasisRecordPolyto output OASIS-format bytes.The two record classes convert geometry information into OASIS-format bytes:
OasisRecordPolyclass accepts coordinate pairs (e.g.,[[410, 720], [410, 920], [70, 920], ...]) and converts them into OASIS-format bytes.OasisRecordRectclass accepts a lower-left coordinate pair (e.g.,[70, 800]), width, height and converts them into OASIS-format bytes.With these record classes,
OasisWriteracceptsOasisRecordPolyandOasisRecordRectobjects and integrates these records with the minimal required headers (e.g., magic-bytes, START, END, CELL, and CELLNAME records) and outputs OASIS-format bytes. The output bytes should be loadable by third-party software.These classes above are wrapped as Python classes via pybind11. In this PR, I added a few unit tests to verify the classes work properly:
OasisRecordPolyandOasisRecordRect: verify that the produced OASIS-format bytes are correct.OasisWriter: verify the output bytes contain the required headers with integrated polygon and rectangle records.In addition, I tested the output bytes manually with KLayout software. The output bytes from
OasisWriteris accepted by KLayout, as shown below.