-
Notifications
You must be signed in to change notification settings - Fork 56
feat: add object detector feature in modmesh #619
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
base: master
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,162 @@ | |||
| /* | |||
| * Copyright (c) 2022, Yung-Yu Chen <[email protected]> | |||
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.
Change to your name if you are the first author.
| QPushButton *toggleBtn = nullptr; | ||
| bool enableDetection = false; | ||
| }; | ||
| std::unique_ptr<Impl> m_impl; |
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.
I don't think we need to use pimpl idiom.
cpp/modmesh/pilot/wrap_pilot.cpp
Outdated
|
|
||
| friend root_base_type; | ||
|
|
||
| WrapRVisionDockWidget(pybind11::module & mod, char const * pyname, char const * pydoc) |
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.
If there is nothing, maybe we don't need this?
modmesh/pilot/_vision.py
Outdated
| logger.setLevel(logging.DEBUG) | ||
|
|
||
| if 'model' not in globals(): | ||
| model = YOLO('./modmesh/pilot/yolo11n.pt') # Please check the model path |
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.
How do you get the model? Maybe have a runtime download logic?
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.
Yes, it will check the path if the model is exist during runtime, nor it will download it directly to the specified path.
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.
I just found there is a directory called thirdparty, maybe I should specify the path overthere instead?
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.
thirdparty is for the 3rd libraries. In this case, I think you can put the model file at the same directory of pilot runtime. Btw, it seems that the download logic is not implemented yet, right?
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.
ultralytics library had already done the download logic, no need to do it agin, perhaps they will find the model name is in their file server or not, then download it when trigger class YOLO initialization.
| RVisionImage::RVisionImage(QWidget *parent) | ||
| : QWidget(parent) | ||
| { | ||
| m_image.load(":/default.jpg"); |
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.
You may want to put the image as an asset file.
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.
If you add an image in the repository, make sure it is small and uses a compatible license.
Alternately, load an image from online.
modmesh/pilot/_vision.py
Outdated
| @@ -0,0 +1,99 @@ | |||
| """ | |||
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.
Is it possible to write tests to validate the implementation?
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.
How would you recommend to place the tests, put them in the tests/test_pilot.py perhaps?
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.
I think you can put it at tests/test_vision.py?
yungyuc
left a comment
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.
Please make sure CI passes before requesting for review.
- Correct copyright headers.
- Remove unnecessary code like
WrapRVisionDockWidget, which looks like a placeholder. - The pimpl class
RVisionDockWidget::Implis not necessary. Do not use pimpl. - Do not create a symbol named
modmesh::BoundingBox. - Always add an end marker to classes and namespaces.
I see you are using pybind11 to call back into Python to use YOLO. Why don't you just write PySide6 to do it?
| }; | ||
|
|
||
| class RVisionDockWidget; | ||
| class Impl; |
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 forward declaration Impl is no-op.
| namespace modmesh | ||
| { | ||
|
|
||
| struct BoundingBox { |
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.
Do not name it as modmesh::BoundingBox, which will be used for other purposes.
Do not create additional namespace under modmesh. It will be over-engineering.
You may make it a nested class in RVisionDockWidget. A shorter name would make the code easier.
| private: | ||
| QImage m_image; | ||
| std::vector<BoundingBox> m_boxes; | ||
| }; |
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.
Always keep an end marker:
...
}; /* end class RVisionImage */| RVisionImage::RVisionImage(QWidget *parent) | ||
| : QWidget(parent) | ||
| { | ||
| m_image.load(":/default.jpg"); |
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.
If you add an image in the repository, make sure it is small and uses a compatible license.
Alternately, load an image from online.
| const uchar *data = rgbImg.bits(); | ||
| py::array_t<uint8_t> np_img({height, width, channels}, data); | ||
| py::object vision_mod = py::module_::import("modmesh.pilot._vision"); | ||
| py::object yolo_func = vision_mod.attr("yolo_detect"); |
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.
If you call YOLO from Python, why not use PySide?
I thought it was a must to write all the GUI component with |
|
@rockleona The code base has changed a lot. Please rebase to refresh the CI status. |
3a678c2 to
79ad5a8
Compare
I started to have a research on computer vision, as the first step, this PR introduce Ultralytics YOLO as the object detection tool, and import yolov11 model as the base model.
Usage is really easy, just load an image, enable the detection, then you will see the result on the Console Widget. You can check the screenshot as below:
