Improve: remember 2D mapper zoom amounts between sessions by SlySven · Pull Request #6615 · Mudlet/Mudlet

SlySven · 2023-03-08T05:39:15Z

This is intended to close #2388.

This enables the 2D (only) map zoom amounts for each map area to be save independently and restored when switching between areas in the mapper. It also saves the data between session.

It extends the existing setMapZoom(...) API to take an optional second argument to specify the map area ID to set the zoom (which is a floating point number) for any existing area, not just the current one.

It also adds a getMapZoom(...) function that, without any arguments, returns the currently used 2D map zoom value for the area currently being shown in the 2D mapper. If an area ID is provided it instead returns the value that was last used for that area - or the default value that is used initially on starting the profile or for an area that has not been viewed before.

Importantly when switching between the areas in the 2D mapper the values are retained and applied so that one area can be zoomed in and another zoomed out and switching from the first to the second and back to the first means that the zoom level used in the first is reused when it is returned to. Deleting an area will forget the stored zoom level so if it is reused it starts from scratch.

Code to save the zoom level for each area has also been implemented within the C++ core. It saves it in the Area User Data for current map formats (but removes it on loading so the user never sees it there) under a system.fallback_map2DZoom key but will save it directly in the binary data (which is more efficient) in the next format version whenever it is enabled.

A new Mudlet event, which has been called mapAreaViewedChangeEvent has been added with two additional arguments being the area ID changed to followed by the one that it was changed from. I originally thought I would need it to handle saving the zoom level for each area via the Lua system but I found that that was not practicable. I'll leave it in for now but it could be removed if peer-review finds it not to be of use.

Also, in refactoring T2DMap::paintEvent(...) I:
Removed/combined some locals:

(TArea*) playerArea and (TArea*) pPlayerArea ==> pArea
(TRoom*) playerRoom ==> pPlayerRoom

Remove unneeded (refactored out):

(qreal) ox
(qreal) oy

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

This is intended to close Mudlet#2388. This enables the 2D (only) map zoom amounts for each map area to be save independently and restored when switching between areas in the mapper. It also saves the data between session. It extends the existing `setMapZoom(...)` API to take an optional second argument to specify the map area ID to set the zoom (which is a floating point number) for any existing area, not just the current one. It also adds a `getMapZoom(...)` function that, without any arguments, returns the currently used 2D map zoom value for the area currently being shown in the 2D mapper. If an area ID is provided it instead returns the value that was last used for that area - or the default value that is used initially on starting the profile or for an area that has not been viewed before. Importantly when switching between the areas in the 2D mapper the values are retained and applied so that one area can be zoomed in and another zoomed out and switching from the first to the second and back to the first means that the zoom level used in the first is reused when it is returned to. Deleting an area will forget the stored zoom level so if it is reused it starts from scratch. Code to save the zoom level for each area has also been implemented within the C++ core. It saves it in the Area User Data for current map formats (but removes it on loading so the user never sees it there) under a `system.fallback_map2DZoom` key but will save it directly in the binary data (which is more efficient) in the next format version whenever it is enabled. A new Mudlet event, which has been called `mapAreaViewedChangeEvent` has been added with two additional arguments being the area ID changed to followed by the one that it was changed from. I originally thought I would need it to handle saving the zoom level for each area via the Lua system but I found that that was not practicable. I'll leave it in for now but it could be removed if peer-review finds it not to be of use. Also, in refactoring `T2DMap::paintEvent(...)` I: Removed/combined some locals: * `(TArea*) playerArea` and `(TArea*) pPlayerArea` ==> `pArea` * `(TRoom*) playerRoom` ==> `pPlayerRoom` Remove unneeded (refactored out): * `(qreal) ox` * `(qreal) oy` Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

add-deployment-links · 2023-03-08T05:39:57Z

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.

Kebap

This does what it says on the tin.
Ideas for further improvement:

Also remember the previous position on map, because right now, switching back and to a map will indeed show the same zoom level, but a different position, if you moved in the intermediate area meanwhile.
Prevent mapper zoom / pan combinations which actually show no rooms at all in the resulting view part
Smartly calculate a "good" default zoom level for each first visit in an area, so it shows the whole map, unless it gets "too big", whatever that means exactly

Kebap

There seems to be something wrong, as setMapZoom(integer) does not change zoom view at all, and:

Kebap · 2023-03-09T07:29:59Z

If I request lua setMapZoom(1, nil) this should set the current area map zoom to 1, right?
Instead, I receive an error informing me about a missing optional argument:
<setMapZoom: bad argument #2 type (area id as number is optional, got nil!)>

SlySven · 2023-03-09T07:47:43Z

There seems to be something wrong, as setMapZoom(integer) does not change zoom view at all, and:

No, the minimum zoom value is 3.0 - anything less than that will produce no effect.

vadi2 · 2023-03-09T07:50:19Z

In that case it should be a Lua error since it'll never be possible to have a zoom level less than 3. What does zoom value 3.0 actually look like though?

SlySven · 2023-03-09T08:05:41Z

If I request lua setMapZoom(1, nil) this should set the current area map zoom to 1, right? Instead, I receive an error informing me about a missing optional argument: <setMapZoom: bad argument #2 type (area id as number is optional, got nil!)>

The second argument is optional, but by explicitly providing a nil you have provided a value - but it is not the correct type (it is a nil not a number) as the error message is reporting.

Remember someone did a lot of refactoring to create and use all those getVerifiedXxxx(...) functions in the TLuaInterpreter class and they included an argument in them to report that the argument that it is extracting a value for from what was passed to the Lua function that the user is invoking is "optional" rather than "required". However those same functions do not have any code in them to check that the "optional" argument was actually provided by the user! 🙄

SlySven · 2023-03-09T08:07:58Z

In that case it should be a Lua error since it'll never be possible to have a zoom level less than 3. What does zoom value 3.0 actually look like though?

That isn't an (argument type) error, it would only be a (run-time, value) warning - and since the previous code did not do anything to report such an issue and behaved in the same way (as far as setting the zoom for the currently viewed area) I didn't feel compelled to add it.

3.0 looks something like this on my particular setup:

That is on a 1920x1080 monitor.

vadi2 · 2023-03-09T08:22:47Z

For backwards compatibility that would make sense. It shouldn't return true though should it?

Kebap · 2023-03-09T09:28:16Z

Remember someone did a lot of refactoring 🙄

What you describe seems like a worthwhile addition (in a new PR).
Indeed I have now compared other existing functions and they show a similar flaw when you provide an optional argument with a nil value.
Then it is OK here for now as well.

SlySven · 2023-03-09T13:27:37Z

For backwards compatibility that would make sense. It shouldn't return true though should it?

Well, given that it has been extended - and there are a number of ways that it can fail and return a nil + error message I think it is not unreasonable to return a true on success. Given that, I now think that it would not be unreasonable to report if the zoom level supplied was too small - that would have given @Kebap a better clue as to what was going on when he tried it and got nothing back when it didn't work. Fortunately that will be a trivial change to do - especially as I parametrised(?) the minimum to a publicly available static symbol (int) T2DMap::csmMinXYZoom! 😀

…l zoom Also revise the error message for it and the corresponding getter `getZoom(...)`, when the versions with an area id provided has an id that doesn't exist, to the same text as some other cases in the Lua API. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Kebap · 2023-03-09T19:49:36Z

src/TLuaInterpreter.cpp

+    }
+
+    if (zoom < T2DMap::csmMinXYZoom) {
+        return warnArgumentValue(L, __func__, qsl("%1 is less than the minimum value allowed for zoom which is %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));


Align language to other warnings in this file.

Suggested change

return warnArgumentValue(L, __func__, qsl("%1 is less than the minimum value allowed for zoom which is %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));

return warnArgumentValue(L, __func__, qsl("zoom %1 is invalid, it must be greater than %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));

Technically it should be "greater than or equal to" or "not less than" - the second is shorter in English, not sure if that negative form is the clearest to use though.

Fair enough.

Suggested change

return warnArgumentValue(L, __func__, qsl("%1 is less than the minimum value allowed for zoom which is %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));

return warnArgumentValue(L, __func__, qsl("zoom %1 is invalid, it must be at least %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));

You're welcome.

Kebap · 2023-03-09T19:49:40Z

src/TLuaInterpreter.cpp

+    if (!result) {
+        // Whilst it will also return false should there not be a map or mapper
+        // we should have already ruled those cases out:
+        return warnArgumentValue(L, __func__, qsl("number %1 is not a valid area id").arg(QString::number(areaID.value())));


as discussed in #2605

Suggested change

return warnArgumentValue(L, __func__, qsl("number %1 is not a valid area id").arg(QString::number(areaID.value())));

return warnArgumentValue(L, __func__, qsl("number %1 is not a valid areaID").arg(QString::number(areaID.value())));

There are multiple instances of both texts in the file IIRC and I didn't do a count of each to find which one was precisely the most common... as it is that issue has not, um, been actioned/resolved yet! However I will take on board that you have pointed it out in regard to these texts in the PR.

Kebap · 2023-03-09T19:49:55Z

src/TLuaInterpreter.cpp

+
+    if (areaID.has_value()) {
+        if (!host.mpMap->mpRoomDB->getArea(areaID.value())) {
+            return warnArgumentValue(L, __func__, qsl("number %1 is not a valid area id").arg(QString::number(areaID.value())));


Suggested change

return warnArgumentValue(L, __func__, qsl("number %1 is not a valid area id").arg(QString::number(areaID.value())));

return warnArgumentValue(L, __func__, qsl("number %1 is not a valid areaID").arg(QString::number(areaID.value())));

Kebap · 2023-03-10T00:14:31Z

The default zoom value seems not very enticing, and short of implementing any smart calculation, I implore you to reconsider this:

I foresee setting zoom per-area being a massive amount of pain since it'll reset between areas on the first time and so on.

If you enter a new area for the first time that does not have a zoom setting yet, start with the previous area's one.

Kebap · 2023-03-10T00:17:41Z

There seems to be something wrong, as setMapZoom(integer) does not change zoom view at all

While I now see a minimum value announced (is there also a maximum?) the zoom value still doesn't change:

Also the area displayed in mapper does not change its zoom levels during this at all.

SlySven · 2023-03-10T14:36:59Z

The default zoom value seems not very enticing, and short of implementing any smart calculation, I implore you to reconsider this:

I foresee setting zoom per-area being a massive amount of pain since it'll reset between areas on the first time and so on.

If you enter a new area for the first time that does not have a zoom setting yet, start with the previous area's one.

Those values are the ones that are already in place, the mapper was starting off the session at 20.0 and 3.0 was the existing limit - therefore they are the least surprising values to use going forward. This PR currently means that if you go to a newly created area (or to one for the first time since you use Mudlet with this PR in place) you will get the default 20.0 zoom - because each area is now handled separately.

Whilst I suppose it might be possible to copy over the zoom from the last area previous viewed I think that that will required extra code and make this PR larger. Would it be reasonable to defer that to a separate PR?

SlySven · 2023-03-10T14:47:15Z

There seems to be something wrong, as setMapZoom(integer) does not change zoom view at all

While I now see a minimum value announced (is there also a maximum?) the zoom value still doesn't change:

{image omitted}

Also the area displayed in mapper does not change its zoom levels during this at all.

There is no maximum limit (well other than an inherent int overflow!)...

Let me check that - I thought it did repaint the map in the new zoom, but maybe I left out an update(); for the case when the area being viewed is the one that the zoom is set for; as it is now possible to set the zoom on a complete different area that is NOT being viewed and that shouldn't prompt a redraw...

SlySven · 2023-03-10T15:43:15Z

Not redrawing map after using single argument setMapZoom(...): Confirmed 🐞 .

Strange thing is that the Lua updateMap() is called (in all cases where the setting is successful yet it is proving ineffectual) furthermore that should only be called in the single argument case or when the area being viewed IS the area given in the two argument case...

I need to dig deeper.

Kebap · 2023-03-10T17:47:37Z

Please convert to draft until new tests are possible

Also revise some error/warning texts. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

SlySven · 2023-03-10T23:58:06Z

Please convert to draft until new tests are possible

Not really necessary - I think it is fixed-up now. Both the messages and the single argument setting case.

…enSessions_simplified

SlySven · 2023-03-11T17:51:51Z

📚 Documentation pending in Area 51:

getMapZoom()
setMapZoom()
~~mapAreaViewedChangeEvent~~ changed to: sysMapAreaChanged

src/T2DMap.cpp

src/T2DMap.h

@Kebap

#### Brief overview of PR changes/additions In particular try and use ID in a uniform manner when referring to an integer number that uniquely identifies an instance of something like a room or an area in the map. #### Motivation for adding to Mudlet It became clear that there are a significant number of duplicate strings involving ID and it was possible to reduce the number of duplicate `qsl(`...`)` - which are our wrapper of `QStringLiteral` - by replacing each with a QString constant initialised at compile time. This is a good thing as it is bad to have multiple copies of the same string as one of these as each one is stored separately in the (Read-Only) data part of the executable. #### Other info (issues closed, discussion etc) This should close #2605. This was prompted by @Kebap 's comments here: #6615 (comment) Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

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

SlySven · 2023-03-16T00:16:50Z

📚 Documentation reordered/improved.

…enSessions_simplified

Work has been done and approval given by project leader.

)" This reverts commit c6e3aa7.

SlySven added enhancement bounty-50 see https://wiki.mudlet.org/w/Bounty_Process labels Mar 8, 2023

SlySven added this to the 4.18.0 milestone Mar 8, 2023

SlySven requested a review from a team as a code owner March 8, 2023 05:39

SlySven assigned vadi2 Mar 8, 2023

SlySven requested review from a team March 8, 2023 05:39

vadi2 changed the title ~~Improve: remember mapper zoom amounts between sessions~~ Improve: remember 2D mapper zoom amounts between sessions Mar 8, 2023

Kebap approved these changes Mar 9, 2023

View reviewed changes

Kebap previously requested changes Mar 9, 2023

View reviewed changes

Kebap added the needs documentation This pull request changes things the players would use/see and thus needs an update in the manual label Mar 9, 2023

Kebap reviewed Mar 9, 2023

View reviewed changes

Fix: get setMapZoom(...) working for single argument case

5a45667

Also revise some error/warning texts. Signed-off-by: Stephen Lyons <slysven@virginmedia.com>

Merge branch 'development' into Improve_rememberMapperZoomAmountBetwe…

d41004a

…enSessions_simplified

SlySven mentioned this pull request Mar 11, 2023

Infrastructure: rationalise strings in TLuaInterpreter class #6636

Merged

SlySven removed the needs documentation This pull request changes things the players would use/see and thus needs an update in the manual label Mar 11, 2023

vadi2 self-requested a review March 12, 2023 07:37

vadi2 reviewed Mar 13, 2023

View reviewed changes

src/T2DMap.cpp Outdated Show resolved Hide resolved

src/T2DMap.cpp Outdated Show resolved Hide resolved

src/T2DMap.cpp Outdated Show resolved Hide resolved

src/T2DMap.h Show resolved Hide resolved

SlySven added 2 commits March 15, 2023 23:29

Revise: some changes suggested in peer review

7e8eb15

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

Revise: another change suggested in peer review

b307d19

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

Merge branch 'development' into Improve_rememberMapperZoomAmountBetwe…

ac48b4e

…enSessions_simplified

vadi2 approved these changes Mar 16, 2023

View reviewed changes

SlySven merged commit c6e3aa7 into Mudlet:development Mar 16, 2023

SlySven deleted the Improve_rememberMapperZoomAmountBetweenSessions_simplified branch March 16, 2023 15:00

vadi2 added a commit that referenced this pull request Mar 18, 2023

Revert "Improve: remember 2D mapper zoom amounts between sessions (#6615

b87bb0d

)" This reverts commit c6e3aa7.

vadi2 added a commit that referenced this pull request Mar 18, 2023

Revert "Improve: remember 2D mapper zoom amounts between sessions (#6615

570350b

)" This reverts commit c6e3aa7.

vadi2 added a commit that referenced this pull request Mar 18, 2023

Revert "Improve: remember 2D mapper zoom amounts between sessions (#6615

d0bedd6

)" This reverts commit c6e3aa7.

vadi2 added a commit that referenced this pull request Mar 18, 2023

Revert "Improve: remember 2D mapper zoom amounts between sessions (#6615

e607ba7

)" This reverts commit c6e3aa7.

	return warnArgumentValue(L, __func__, qsl("%1 is less than the minimum value allowed for zoom which is %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));
	return warnArgumentValue(L, __func__, qsl("zoom %1 is invalid, it must be greater than %2").arg(QString::number(zoom), QString::number(T2DMap::csmMinXYZoom)));

Uh oh!

Conversation

SlySven commented Mar 8, 2023

Uh oh!

add-deployment-links bot commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test versions

Uh oh!

Kebap left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kebap left a comment

Choose a reason for hiding this comment

Uh oh!

Kebap commented Mar 9, 2023

Uh oh!

SlySven commented Mar 9, 2023

Uh oh!

vadi2 commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SlySven commented Mar 9, 2023

Uh oh!

SlySven commented Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vadi2 commented Mar 9, 2023

Uh oh!

Kebap commented Mar 9, 2023

Uh oh!

SlySven commented Mar 9, 2023

Uh oh!

Kebap Mar 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SlySven Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

Kebap Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

Kebap Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

SlySven Mar 10, 2023

Choose a reason for hiding this comment

Uh oh!

Kebap Mar 9, 2023

Choose a reason for hiding this comment

Uh oh!

Kebap commented Mar 10, 2023

Uh oh!

Kebap commented Mar 10, 2023

Uh oh!

SlySven commented Mar 10, 2023

Uh oh!

SlySven commented Mar 10, 2023

Uh oh!

SlySven commented Mar 10, 2023

Uh oh!

Kebap commented Mar 10, 2023

Uh oh!

SlySven commented Mar 10, 2023

Uh oh!

SlySven commented Mar 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

SlySven commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

add-deployment-links bot commented Mar 8, 2023 •

edited

Loading

Kebap left a comment •

edited

Loading

vadi2 commented Mar 9, 2023 •

edited

Loading

SlySven commented Mar 9, 2023 •

edited

Loading

Kebap Mar 9, 2023 •

edited

Loading

SlySven commented Mar 11, 2023 •

edited

Loading