Skip to content

Mapper: draw room exits that go off the map#4716

Merged
SlySven merged 1 commit intoMudlet:developmentfrom
SlySven:BugFix_correctlyEvaluateRoom_m(in|ax)(x|y)_valuesInAllCases
Jan 30, 2021
Merged

Mapper: draw room exits that go off the map#4716
SlySven merged 1 commit intoMudlet:developmentfrom
SlySven:BugFix_correctlyEvaluateRoom_m(in|ax)(x|y)_valuesInAllCases

Conversation

@SlySven
Copy link
Copy Markdown
Member

@SlySven SlySven commented Jan 30, 2021

We previously set them all to be 0.0 when there is no custom lines but technically, since they are supposed to be extremes of the room and it's custom exit lines they should be initialised to the room's own coordinates.

Not doing this meant that in some cases they were being used but they did not hold sensible values.

Signed-off-by: Stephen Lyons slysven@virginmedia.com

We previously set them all to be 0.0 when there is no custom lines but
technically, since they are supposed to be extremes of the room and it's
custom exit lines they should be initialised to the room's own coordinates.

Not doing this meant that in some cases they were being used but they
did not hold sensible values.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven added this to the 4.11.0 milestone Jan 30, 2021
@SlySven SlySven requested a review from a team as a code owner January 30, 2021 00:04
@SlySven SlySven requested a review from a team January 30, 2021 00:04
@add-deployment-links
Copy link
Copy Markdown

add-deployment-links bot commented Jan 30, 2021

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.

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Jan 30, 2021

This will fix the bug remarked upon in #4608 and close #3780 .

@SlySven SlySven linked an issue Jan 30, 2021 that may be closed by this pull request
@vadi2 vadi2 changed the title BugFix: correctly evaluate room m(in|ax)(x|y) values in all cases Mapper: draw room exits that go off the map Jan 30, 2021
qreal _y = point.y();
if (_x < min_x) {
min_x = _x;
for (auto pointInLine : pointsInLine) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know the old code did this - but when you use Qt containers their copy-on-write behaviour can trigger accidentally and make a copy list - doesn't hurt the data but hurts performance. Have to wrap the access in a qAsConst(). std:: ones don't suffer from this btw

Suggested change
for (auto pointInLine : pointsInLine) {
for (auto pointInLine : qAsConst(pointsInLine)) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Um, pointsInLine is a const reference already and it was obtained from a QMap<QString, QList<QPointF>>::value() which is, IIRC a copy of the original data - so Qt's CoW actions cannot be triggered here AFAICT.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

IMVHO If you feel strongly enough about this then we can change this in another PR.

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Jan 30, 2021

Fixed up the title to something testers can understand using the PTBs. Remember to keep them simple

@SlySven SlySven merged commit 80284c2 into Mudlet:development Jan 30, 2021
@SlySven SlySven deleted the BugFix_correctlyEvaluateRoom_m(in|ax)(x|y)_valuesInAllCases branch January 30, 2021 13:50
Chris7 pushed a commit to Chris7/Mudlet that referenced this pull request Jan 2, 2022
Retitled from:
"BugFix: correctly evaluate room m(in|ax)(x|y) values in all cases"

We previously set them all to be 0.0 when there is no custom lines but
technically, since they are supposed to be extremes of the room and it's
custom exit lines they should be initialised to the room's own coordinates.

Not doing this meant that in some cases they were being used but they
did not hold sensible values.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Map - not always show links between locations

3 participants