Improve: speedup viewing the 2D map #7189

vadi2 · 2024-03-17T09:52:09Z

Brief overview of PR changes/additions

Speedup viewing/panning the map - depending on the map zoom level, from 15% to 150% speedup improvement

Motivation for adding to Mudlet

A million rooms makes the 2D mapper somewhat slow

Other info (issues closed, discussion etc)

https://discord.com/channels/283581582550237184/1218643821311823983/1218813662534303754

Delete extra setPen calls

We weren't actually using them for a gain

add-deployment-links · 2024-03-17T09:52:48Z

Hey there! Thanks for helping Mudlet improve. 🌟

Test versions

You can directly test the changes here:

No need to install anything - just unzip and run.
Let us know if it works well, and if it doesn't, please give details.

src/T2DMap.cpp

vadi2 · 2024-03-29T14:06:36Z

src/TRoomDB.cpp

-        return i.value();
-    }
-    return nullptr;
+    return rooms.value(id, nullptr);


Qt's own implementation was quicker.

demonnic

looks good to me and functional testing bears out

src/CMakeLists.txt

SlySven

I haven't finished reviewing this - I'm just going out and will be changing PCs...

SlySven · 2024-04-06T14:12:59Z

src/T2DMap.cpp

    if (shouldDrawBorder && mRoomWidth >= 12) {
-        roomPen.setColor(mpHost->mRoomBorderColor);
+        pen.setColor(mpHost->mRoomBorderColor);
    } else if (shouldDrawBorder) {
        auto fadingColor = QColor(mpHost->mRoomBorderColor);
        fadingColor.setAlpha(255 * (mRoomWidth / 12));
-        roomPen.setColor(fadingColor);
-    }
-
-    if (isRoomSelected) {
+        pen.setColor(fadingColor);
+    } else if (roomSelected) {
        QLinearGradient selectionBg(roomRectangle.topLeft(), roomRectangle.bottomRight());
        selectionBg.setColorAt(0.2, roomColor);
        selectionBg.setColorAt(1, Qt::blue);
-        roomPen.setColor(QColor(255, 50, 50));
+        pen.setColor(QColor(255, 50, 50));
        painter.setBrush(selectionBg);
+    } else {
+        pen.setColor(Qt::transparent);
    }


I'm not sure the logic has stayed the same here - it looks as though you are combining the boarder drawing with the room selection drawing - and I'm not entirely sure that is the case. OTOH I didn't do the room borders so I might be missing something...

😕

src/T2DMap.cpp

SlySven · 2024-04-06T14:42:17Z

src/TRoomDB.cpp

-        return i.value();
-    }
-    return nullptr;
+    return rooms.value(id, nullptr);


The second argument is unneeded in this usage - https://doc.qt.io/qt-5/qhash.html#value :

Suggested change

return rooms.value(id, nullptr);

return rooms.value(id);

I've seen that link and I am unsure why you think this. The default is not a null pointer but a default-constructed value.

A default constructed value in this case is a nullptr! Remember that the (QHash<K,V>) rooms has a <int, TRoom*> type...

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

SlySven

Okay, I've looked through the code but I haven't tested it yet - I want to be sure the room drawing code refactoring produces the same output (uses the right pen for each thing) - but I'll need to get back onto my main PC to check it later on (I'm out of home at the moment).

SlySven · 2024-04-06T17:31:00Z

src/CMakeLists.txt

@@ -544,7 +544,7 @@ target_link_libraries(
        ZIP::ZIP
        ZLIB::ZLIB
 # PLACEMARKER: sample benchmarking code
-#       nanobench
+#         nanobench


Dammit! The spacing looks to have been off in my suggestion...

SlySven · 2024-04-06T17:31:50Z

src/T2DMap.cpp

-// This has been refactored to a separate function out of the paintEven() code
-// because we need to use it in two places - one for every room that is not the
-// player's room and then, AFTER all those have been drawn, once for the
-// player's room if it is visible. This is so it is drawn LAST (and any effects,
-// or extra markings for it do not get overwritten by the drawing of the other
-// rooms)...


@vadi2 You think this is unhelpful then?

SlySven · 2024-04-06T17:35:37Z

src/T2DMap.h

@@ -254,12 +254,12 @@ public slots:
    std::pair<int, int> getMousePosition();
    bool checkButtonIsForGivenDirection(const QPushButton*, const QString&, const int&);
    bool sizeFontToFitTextInRect(QFont&, const QRectF&, const QString&, const quint8 percentageMargin = 10, const qreal minFontSize = 7.0);
-    void drawRoom(QPainter&, QFont&, QFont&, QPen&, TRoom*, const bool isGridMode, const bool areRoomIdsLegible, const bool showRoomNames, const int, const float, const float, const QMap<int, QPointF>&);
+    void drawRoom(QPainter&, QFont&, QFont&, QPen&, QPen&, QPen&, QBrush&, QBrush&, TRoom*, const bool isGridMode, const bool areRoomIdsLegible, const bool showRoomNames, const int, const float, const float, const QMap<int, QPointF>&);


💡 Can you nano bench this to see if an inline (effectively cutting out one method call in the room interation) improves things at all?

SlySven · 2024-04-06T17:38:08Z

src/T2DMap.h

    void paintMapInfo(const QElapsedTimer& renderTimer, QPainter& painter, const int displayAreaId, QColor& infoColor);
    int paintMapInfoContributor(QPainter&, int xOffset, int yOffset, const MapInfoProperties& properties);
-    void paintRoomExits(QPainter&, QPen&, QList<int>& exitList, QList<int>& oneWayExits, const TArea*, int, float, QMap<int, QPointF>&);
+    void paintRoomExits(QPainter&, QPen&, QList<int>& exitList, QList<int>& oneWayExits, const TArea*, int, float, QMap<int, QPointF>&) const;


💡 Like drawRoom(...) (and drawDoor(...) this might produce an improvement if it were inlined - if the compiler doesn't anyhow - but it might need testing to confim.

SlySven · 2024-04-07T03:22:03Z

Um, the refactoring has messed up some things - these are some screenshots at maximum size (zoomed in as much as possible) on a room with two-way exits north and south, a two-way east exit with an open door, a west stub exit, an up exit with an "open" door and a down exit with a "closed" door and an in stub exit with a "locked" door:

Original plain room:

After this PR plain room:

So it looks as though the "oversized" black (IIRC it'll be white on a dark room colour) background under the door line is being missed off of the first door being drawn (the order of drawing is up, down, in and then out) - and it starts to be done at line 914 in T2DMap.cpp now.

Original selected room (no boarders)

After this PR selected room (no boarders):

Notice how the oversized line under the up exit marking triangle is now in the "selected" orange colour instead of being black (or white) here.

Original room (with borders):

After this PR room (with borders):

Same problem - and it'll be probably fixed by the same change!

Original room selected (with borders):

After this PR room selected (with borders):

Not only is the boarder not in the orange "selected" colour, the gradient on the room colour has gone!

Original room with highlight (with borders):

After this PR room with highlight (with borders):

There is a circular border on the highlight now - there should not be!

Original rooms showing room numbers:

After this PR rooms showing room numbers:

It looks as though the (contrasting black or white depending on room colour) colour for the room number is no longer being used.

I did not spot any addition discrepancies with the drawing of the room symbol
Original showing room symbol AB:

After this PR showing room symbol AB:

SlySven · 2024-04-07T03:47:27Z

(QPen) roomBorderPen is defined and passed to T2DMap::drawRoom(...) but is not used also const int borderWidth is calculated therein but is not currently being used (it is also calculated - and used - in the main T2DMap::paintEvent(...)).

BTW IMHO It is not necessarily a good idea to use the same variable names inside a method as the values that passed in from the caller - although in this case it made it easy to tell this one is not used (which is probably a problem) and the Qt "refactor" to function "helper" does it like that - it makes it harder to tell that a variable is not a member if those are NOT being marked as we have been, but not invariably, in some way.

Vadim Peretokin added 6 commits March 17, 2024 09:34

Performance improvements so far

2402a77

Re-use pens for drawing rooms

2e0976e

Remove another pen

cb47083

Don't set pen to transparent when it'll be overridden later

44c6459

Delete extra setPen calls

Remove painter save/restore

042f373

We weren't actually using them for a gain

Disable benchmarking code

7776ae0

Vadim Peretokin added 3 commits March 17, 2024 11:04

Don't include nanobench, qmake breaks

e09e127

Re-use brushes

a996558

Add border width for pen

36f5de2

vadi2 marked this pull request as ready for review March 29, 2024 13:48

vadi2 requested a review from a team as a code owner March 29, 2024 13:48

Merge branch 'development' into speedup-map-panning

1347d87

vadi2 requested review from a team as code owners March 29, 2024 13:48

vadi2 marked this pull request as draft March 29, 2024 13:54

Code cleanup

5e84fc7

vadi2 commented Mar 29, 2024

View reviewed changes

src/T2DMap.cpp Show resolved Hide resolved

vadi2 commented Mar 29, 2024

View reviewed changes

src/T2DMap.cpp Show resolved Hide resolved

vadi2 commented Mar 29, 2024

View reviewed changes

src/TRoomDB.cpp

return i.value();

}

return nullptr;

return rooms.value(id, nullptr);

Copy link

Member Author

vadi2 Mar 29, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Qt's own implementation was quicker.

Final cleanup

0bf8ed9

vadi2 requested a review from SlySven March 29, 2024 14:06

vadi2 marked this pull request as ready for review March 29, 2024 14:07

demonnic approved these changes Mar 29, 2024

View reviewed changes

SlySven reviewed Apr 6, 2024

View reviewed changes

src/CMakeLists.txt Outdated Show resolved Hide resolved

SlySven reviewed Apr 6, 2024

View reviewed changes

Update src/CMakeLists.txt

865b8d4

Co-authored-by: Stephen Lyons <slysven@virginmedia.com>

SlySven reviewed Apr 6, 2024

View reviewed changes

SlySven mentioned this pull request Apr 14, 2024

highlight() is drawn below some rooms. #7206

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve: speedup viewing the 2D map #7189

Improve: speedup viewing the 2D map #7189

vadi2 commented Mar 17, 2024 •

edited

add-deployment-links bot commented Mar 17, 2024 •

edited

vadi2 Mar 29, 2024

demonnic left a comment

SlySven left a comment

SlySven Apr 6, 2024

SlySven Apr 6, 2024

vadi2 Apr 6, 2024

SlySven Apr 6, 2024 •

edited

SlySven left a comment

SlySven Apr 6, 2024

SlySven Apr 6, 2024

SlySven Apr 6, 2024

SlySven Apr 6, 2024

SlySven commented Apr 7, 2024

SlySven commented Apr 7, 2024

Improve: speedup viewing the 2D map #7189

Are you sure you want to change the base?

Improve: speedup viewing the 2D map #7189

Conversation

vadi2 commented Mar 17, 2024 • edited

Brief overview of PR changes/additions

Motivation for adding to Mudlet

Other info (issues closed, discussion etc)

add-deployment-links bot commented Mar 17, 2024 • edited

Test versions

Choose a reason for hiding this comment

demonnic left a comment

Choose a reason for hiding this comment

SlySven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlySven Apr 6, 2024 • edited

Choose a reason for hiding this comment

SlySven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SlySven commented Apr 7, 2024

SlySven commented Apr 7, 2024

vadi2 commented Mar 17, 2024 •

edited

add-deployment-links bot commented Mar 17, 2024 •

edited

SlySven Apr 6, 2024 •

edited