Conversation
Add to OpenBoard multi touch drawing. It allows several people to draw at once with tool Pen and Marker.
|
@ThomasLucky13 has been working very hard on this for quite a long time, and the resulting code looks tine and beautiful ;) |
Vekhir
left a comment
Vekhir
left a comment
There was a problem hiding this comment.
Thanks for your contribution, I believe that multi-touch is a useful feature to have.
I'm not that familiar with Qt's multitouch support, so I have some general remarks which you might use as an opportunity to expand on your design choices.
src/domain/UBGraphicsScene.cpp
Outdated
| int distance = sqrt(pow((lastPoint_m.x() - endPoint_m.x()),2) + pow((lastPoint_m.y() - endPoint_m.y()),2)) + 1; | ||
| distance = sqrt(distance); | ||
| if (distance > 6) | ||
| distance = 6; | ||
| else if(distance < 4) | ||
| distance = 4; |
There was a problem hiding this comment.
What is distance used for? It's defined here but never used.
| lastPoint_m = point.lastPos(); | ||
| endPoint_m = point.pos(); |
There was a problem hiding this comment.
Is there a reason to not have lastPoint_m and endPoint_m as local variables if they are reassigned every time?
src/domain/UBGraphicsScene.cpp
Outdated
| addPolygonItemToCurrentStroke(polygonItem); | ||
| } | ||
|
|
||
| lastPoint_m = endPoint_m; |
There was a problem hiding this comment.
Does this have any effect? It's overwritten later, see above.
| if (!multiDrawLines.contains(line)) // to eliminate duplicates | ||
| { | ||
| multiDrawLines.append(line); |
There was a problem hiding this comment.
In terms of performance, QList::contains has linear runtime in the number of lines contained, especially if the line isn't in the list. Are duplicates a serious issue?
There was a problem hiding this comment.
This is important because there are so many duplicates that the page can take a long time to load
There was a problem hiding this comment.
If we are spending so much time on searching rather than appending, it might be worth using QSet instead of QList. QSet requires a bit more memory, but look-up is really fast.
Can you test it with QSet and report back whether it makes an appreciable difference in performance? Otherwise I'd just leave it as is.
src/domain/UBGraphicsScene.cpp
Outdated
| addPolygonItemToCurrentStroke(poly); | ||
| } | ||
|
|
||
| if(multiDrawLines.isEmpty()) // is it not polygons drawing by multiDraw |
There was a problem hiding this comment.
Please use braces and indentation to better indicate the scope.
Thank you for code review. I was careless in some places and corrected them. |
clear code after code review by Vekhir
ThomasLucky13
force-pushed
the
multi-touch
branch
from
d5c22fa to
f42ce1e
Compare
| return mItemCount == 0; | ||
| } | ||
|
|
||
| bool UBGraphicsScene::eventFilter(QObject *watched, QEvent *event) |
There was a problem hiding this comment.
Is there a reason to use an eventFilter in the scene instead of overriding event() in the view? I would assume (but have not tested!) that the touch events arrive at the view and can be handled there. If this is the case, then I would also move the event handling to the view, while the multi-touch drawing code could be here in the scene.
What you're touching with your finger is a screen with a widget, which is the view. You don't "touch" the scene. The scene just holds all the graphics items.
src/domain/UBGraphicsScene.cpp
Outdated
| { | ||
| QList <QTouchEvent::TouchPoint> touchPoints = event->touchPoints(); | ||
| QPointF lastPoint_m, endPoint_m; | ||
| foreach (QTouchEvent::TouchPoint point, touchPoints) |
There was a problem hiding this comment.
Qt6 recommends to prefer the C++ range based for over the foreach keyword. See https://doc.qt.io/qt-6/foreach-keyword.html. It is also recommended to use a const reference in a case like this to avoid copying the point.
1. Remove 'if' construct from releaseAllInputDevices() in UBBoardView; 2. Rename MultiTouchDrawing to multiTouchDrawing and MultiTouchEndDrawing to multiTouchEndDrawing; 3. Make point const in multiTouchDrawing.
5 participants
Add to OpenBoard multi touch drawing. It allows several people to draw at once on Board mode with tools Pen and Marker.