pilot: Extract shared paint_world_2d render routine (issue #754 PR1)#899
pilot: Extract shared paint_world_2d render routine (issue #754 PR1)#899tigercosmos wants to merge 1 commit into
Conversation
…PR1)
Move the body of R2DWidget::paintWorld into a free paint_world_2d routine in new RPainter2d.{hpp,cpp} so the on-screen widget and a future offscreen image renderer share one code path. Pure refactor; pixels unchanged.
tigercosmos
left a comment
tigercosmos
left a comment
There was a problem hiding this comment.
@yungyuc Please take a look, thanks!
|
|
||
| } // unnamed namespace | ||
|
|
||
| void paint_world_2d(QPainter & painter, WorldFp64 const & world, ViewTransform2dFp64 const & view) |
There was a problem hiding this comment.
Refactor the function to here. QPainter can take the argument from R2DWidget or future QImage
| { | ||
| paint_world_2d(painter, *m_world, m_view); | ||
| } |
There was a problem hiding this comment.
paint_world_2d is called here.
yungyuc
left a comment
yungyuc
left a comment
There was a problem hiding this comment.
LGTM. If you do not want to enhance the architecture, I can just merge. Let me know what I should do.
| set(MODMESH_PILOT_PYMODHEADERS | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/R3DWidget.hpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/R2DWidget.hpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/RPainter2d.hpp |
There was a problem hiding this comment.
I like the namd RPainter2d. The names of R3DWidget and R2DWidget should follow. But "widget" is not informative. Do not rename hastily. Let's give it more thoughts.
The only complain I have with "painter" is that it sounds like a pixel facility, like the Windows legacy suggested. But it's not a big deal. Just to share an instinctive feeling.
There was a problem hiding this comment.
I changed to RWorldRenderer2d.
| constexpr double GEOMETRY_LINE_WIDTH_PX = 1.5; | ||
| constexpr int GEOMETRY_POINT_WIDTH_PX = 5; | ||
|
|
||
| } // unnamed namespace |
There was a problem hiding this comment.
It's called anonymous namespace. "Unnamed" is unconventional.
| void paint_world_2d(QPainter & painter, WorldFp64 const & world, ViewTransform2dFp64 const & view) | ||
| { | ||
| // Map math-convention world (x, y) to Qt screen pixels; z is dropped. | ||
| auto map = [&view](double world_x, double world_y) |
There was a problem hiding this comment.
When a lambda/closure is used but not for a functional purpose, it is a sign that the enclosing code should be made as a class and the lambda should be a member function.
But a function like this size (paint_world_2d) is short enough and readable. It's OK to write code like this.
A more extensible design is to make it a class now. By holding WorldFp64 & as a required member datum, the architecture explains itself.
There was a problem hiding this comment.
Sure, let's make it a class directly.
2 participants
The 2D world-drawing logic currently lives inside R2DWidget::paintWorld, where it closes over the widget's
thisandm_view. The offscreen QImage renderer planned next (PR2) needs that same drawing without an event loop or a widget instance, so this PR extracts the body verbatim into a free function paint_world_2d(QPainter&, WorldFp64 const&, ViewTransform2dFp64 const&) in new RPainter2d.{hpp,cpp}. R2DWidget::paintEvent now guards on m_world and calls the shared routine, which keeps the on-screen path and the future offscreen path from drifting apart. The geometry color and cosmetic line/point widths move with the code into RPainter2d's anonymous namespace. This is a pure refactor: paintEvent still draws the background, grid, axes, and origin marker itself and invokes the world geometry at the same point in the same order, so the rendered pixels are unchanged.To let the shared routine take the world by const reference, World::points() is made const. It already returned a const reference to the shared pointer, so this only widens where it can be called and breaks no existing caller.
Built the pilot, ran the pilot Python tests (test_pilot.py and test_pilot_2d.py both green), and ran the C++ lint set (cformat, cinclude, checkascii, checktws) plus flake8 -- all clean. The two test docstrings in test_pilot_2d.py that referenced the old paintWorld name are updated to paint_world_2d.
Related to #754.