Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhance: improve code for room custom exit lines in 2D map #2106

Conversation

SlySven
Copy link
Member

@SlySven SlySven commented Nov 5, 2018

Changes the TRoom class to store the detail about the style of line as the Qt::PenStyle enum that is used when it is actually drawn (saves a few bytes per custom exit line)! Concurrently change the 2D map UI that adjusts this setting on both new custom exit lines being drawn and existing lines selected by the UI to separate the text used to describe the style from this value so that the text can be translated for other GUI languages without breaking the code. Enhance the control (a QComboBox) used for this so that it has icons with a visual representation of each style!

Revise the Lua function addCustomLine to bring the error reporting up to current UI style - including detection some error conditions that were not previously reported (included whether there was the exit that the custom line was to be added to show). Also the target room who's location was used as the end-point for a single segment line was not being checked to confirm that it existed and was in the same area as the room that the exit line was being drawn from. Alternatively, when a table of tables of coordinate triplets {x, y, z} was provided there was previously no proper validation that they all existed and were numbers to be used as coordinates...

I also discovered that TLuaInterpreter::dirToString was not producing the right QStrings necessary for numeric arguments for the normal exit direction to work in the two lua functions that used it addCustomLine and setExitWeight) I have corrected it and updated the callers of it to now return the right normal exit strings needed for each of them.

To validate whether there is actually an exit in the direction that the lua addCustomLine function is told is a little complicated so I have added a (bool) TRoom::hasExitOrSpecialExit(const QString&, const bool) const method that does this so that addCustomLine can return a run-time error (nil + error message) if the given exit does not already exist.

In some places in the TLuaInterpreter custom line functions and the T2DMap::paintEvent() method non-const method were being used to access the details of the custom exit lines - as this takes longer and runs the risk of changing the data when it should not be being changed I have switched to using the read only or constant at(...)/value(...) methods rather than the read/write or non-constant operator[...] method where practical.

I have also made each custom exit line be drawn as a polyline rather than a series of single segments - this is more efficient I think and it means that the dotted/dashed pattern effect "goes around" each corner rather than restarting on each segment which looks better IMHO - draw a multi-segment
line and move one of the vertexes towards the end and you will see the differences. 8-)

This should close #2095 !

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

Changes the TRoom class to store the detail about the style of line as the
Qt::PenStyle enum that is used when it is actually drawn (saves a few
bytes per custom exit line)!  Concurrently change the 2D map UI that
adjusts this setting on both new custom exit lines being drawn and existing
lines selected by the UI to separate the text used to describe the style
from this value so that the text can be translated for other GUI languages
without breaking the code.  Enhance the control (a `QComboBox`) used for
this so that it has icons with a visual representation of each style!

Revise the Lua function `addCustomLine` to bring the error reporting up to
current UI style - including detection some error conditions that were not
previously reported (included whether there was the exit that the custom
line was to be added to show). Also the target room who's location was used
as the end-point for a single segment line was not being checked to confirm
that it existed and was in the same area as the room that the exit line was
being drawn from. Alternatively, when a table of tables of coordinate
triplets {x, y, z} was provided there was previously no proper validation
that they all existed and were numbers to be used as coordinates...

I also discovered that TLuaInterpreter::dirToString was not producing the
right `QString`s necessary for numeric arguments for the normal exit
direction to work in the two lua functions that used it (`addCustomLine`
and `setExitWeight`) I have corrected it and updated the callers of it to
now return the right normal exit strings needed for each of them.

To validate whether there is actually an exit in the direction that the
lua addCustomLine function is told is a little complicated so I have added
a `(bool) TRoom::hasExitOrSpecialExit(const QString&, const bool) const`
method that does this so that addCustomLine can return a run-time error
(nil + error message) if the given exit does not already exist.

In some places in the TLuaInterpreter custom line functions and the
`T2DMap::paintEvent()` method non-const method were being used to access
the details of the custom exit lines - as this takes longer and runs the
risk of changing the data when it should not be being changed I have
switched to using the read only or constant `at(...)`/`value(...)` methods
rather than the read/write or non-constant `operator[...]` method where
practical.

I have also made each custom exit line be drawn as a polyline rather than
a series of single segments - this is more effiecent I think and it means
that the dotted/dashed pattern effect "goes around" each corner rather than
restarting on each segment which looks better IMHO - draw a multi-segment
line and move one of the vertexes towards the end and you will see the
differences. 8-)

This should close Mudlet#2095 !

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven requested a review from a team November 5, 2018 23:29
@SlySven SlySven requested review from a team as code owners November 5, 2018 23:29
@vadi2 vadi2 added this to the 3.16.0 milestone Nov 6, 2018
Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

The code looks amazing! I only have trivial complaints.

src/TLuaInterpreter.h Outdated Show resolved Hide resolved
src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved
src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved
int area_to = pR_to->getArea();
if (area != area_to) {
lua_pushnil(L);
lua_pushfstring(L, "target room is in a different area \"%s\" (id: %d) than the one \"%s\" (id: %d) that has the room to which this custom line is to be added",
Copy link
Member

Choose a reason for hiding this comment

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

I found that a bit difficult to read, how about:

target room is in a different area "%s" (id: %d) compared to the starting room's area "%s" (id: %d)

Copy link
Member Author

Choose a reason for hiding this comment

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

I get what you mean - I was trying to avoid the suggestion that the target room actually represented the room that the exit goes to - it probably does in practice but it is not within the remit of this function to make the actual exit route be to the target room. I'll have a think about this and see if I can improve the readability of the text a bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you think now? 🙂

lua_pushstring(L, "addCustomLine: Coordinate list must be a table of tabled coordinates");
lua_error(L);
return 1;
lua_pushfstring(L, "addCustomLine: bad argument #2 table inner table %d type {coordinate list must be a table containing sets (tables) with three coordinates, indicated inner table is a %s!)",
Copy link
Member

Choose a reason for hiding this comment

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

Instead of "indicated inner table is a %s!" how about the usual "got %s!" ? Because if the inner table isn't a table, saying the table is something else is a little odd.

Copy link
Member Author

Choose a reason for hiding this comment

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

It *is a table isn't it, for example in the following:

addCustomExit(1, {{3, 3, 0}, {3, 6, 0}, {6, 6, 0}, {9, 9, 0}}, "E", "dash dot dot line", {64, 255, 255}, true)

The %d indexes that table, i.e in this case it selects:
1 = {3, 3, 0}
2 = {3, 6, 0}
3 = {6, 6, 0}
4 = {9, 9, 0}
so that error message is saying that instead of being a table of three coordinates {x, y, z} that item is something else (the %s) and, for instance if the command given was:

addCustomExit(1, {{3, 3, 0}, {3, 6, 0}, "something else", {9, 9, 0}}, "E", "dash dot dot line", {64, 255, 255}, true)

the message would be:

addCustomLine: bad argument #2 table inner table 3 type {coordinate list must be a table containing sets (tables) with three coordinates, indicated inner table is a string!)

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, yeah, that is a bit awkward 🙁 - not sure how we can phrase that, how about:

addCustomLine: bad argument #2 table item #%d type {coordinate list as table containing tables (of three coordinates) expected, but got %s at that index!)

Copy link
Member

Choose a reason for hiding this comment

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

That's a good one

for (int i = 1, total = z.size(); i < total; ++i) {
if (lz != z.at(i)) {
lua_pushnil(L);
lua_pushfstring(L, "the z values are not all on the same level (first wrong value is at index %d)", i+1);
Copy link
Member

Choose a reason for hiding this comment

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

Adding in the value itself would help.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay - will do.

lua_pushboolean(L, true);
// Better refresh the 2D map to show the new line:
if (host.mpMap->mpMapper) {
if (host.mpMap->mpMapper->mp2dMap) {
Copy link
Member

Choose a reason for hiding this comment

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

&& ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, that would work - it was just I copied the code from somewhere where there was another inner if IIRC.

pR->customLinesStyle[direction] = line_style;
pR->customLinesColor[direction] = colors;

lua_pushboolean(L, true);
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer the push to be right above the return so let's less confusing to read top-to-bottom.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, will revise.

src/TRoom.cpp Outdated Show resolved Hide resolved
@@ -57,7 +57,7 @@
</sizepolicy>
</property>
<property name="toolTip">
<string>Select Style, Color and whether to end the line with an arrow head BEFORE then choosing the exit to draw the line for...</string>
<string>&lt;html&gt;&lt;head/&gt;&lt;body&gt;&lt;p&gt;Select Style, Color and whether to end the line with an arrow head BEFORE then choosing the exit to draw the line for...&lt;/p&gt;&lt;/body&gt;&lt;/html&gt;</string>
Copy link
Member

Choose a reason for hiding this comment

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

Accidental html rubbish? Same with others below. We only need the <p> to get rich text.

Copy link
Member Author

Choose a reason for hiding this comment

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

Not my choice - it is the automagically inserted Qt Designer plugin output - at least it is not as bad as it used to be. 😨

Let me see if I can clean it up by hand. 🛠️

Copy link
Member Author

Choose a reason for hiding this comment

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

Have put just the <p>...</p> around all tooltips in src/ui/custom_lines.ui and src/ui/custom_line_properties.ui.

@vadi2 vadi2 assigned keneanung and SlySven and unassigned vadi2 Nov 6, 2018
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven assigned vadi2 and unassigned SlySven Nov 7, 2018
@SlySven
Copy link
Member Author

SlySven commented Nov 7, 2018

@vadi2 any better now?

src/TLuaInterpreter.h Outdated Show resolved Hide resolved
(host.mpMap->mpRoomDB->getAreaNamesMap()).value(area_to).toUtf8().constData(), area_to,
(host.mpMap->mpRoomDB->getAreaNamesMap()).value(area).toUtf8().constData(), area);
lua_pushfstring(L,
"target room is in area \"%s\" (id: %d) which is not the one \"%s\" (id: %d) in which this custom line is to be drawn",
Copy link
Member

Choose a reason for hiding this comment

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

Mmm how about this?

"target room is in area "%s" (id: %d) which is not the same one "%s" (id: %d) from which this custom line is to be drawn"

Copy link
Contributor

Choose a reason for hiding this comment

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

"target room is in a different area (called "%s" with id: %d) than the area from which this custom line is to be drawn (called "%s" with id: %d)"

Copy link
Member

Choose a reason for hiding this comment

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

I don't really see a need for such verbosity... how come it appeals to you?

Copy link
Contributor

Choose a reason for hiding this comment

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

If find "not the same one" more verbose than "different", other verbosity is to increase clarity for non-English speakers

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need to spell out called "%s" with id: "%d", people will be familiar with the values so they'll understand what they mean.

src/TRoom.cpp Outdated

// Now check the normal ones:
if (isForCustomExitLines) {
if (text == QLatin1String("N")) {
Copy link
Member

Choose a reason for hiding this comment

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

Could you uppercase text? Because otherwise using down or Down will be considered invalid by this check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not understand what you are concluding! 😕

If the user provides "DOWN" as the exit direction when they are using addCustomLine (or "down" for setExitWeight if I had remembered to update the code for that to use this function as well
🙈 🔨) then this will return whatever TRoom::hasExit(DIR_DOWN) produces - which should be true or false depending on whether there is a normal exit in that (down) direction....

If you mean that using "down" when adding a custom exit line or - once, I tweak it, "DOWN" for when setting an exit weigh not being recognised as the normal exit in that direction, then yes, that is intentional: only the exact case match as listing in the Wiki will match so that a Special Exit - which is case sensitive (so there can be a Special exit "Down" with, in this case an initial capital letter) can be detected and matched. Of course this difficulty/ambiguity/variation in the arguments for different lua functions when it comes to identifying and distinguishing between normal and special exits is a side-effect of the piecemeal development of features over time and the difficulty of maintaining backwards compatibility. Eventually we may have to declare some commands as deprecated and provide replacements with a more uniform methodology in specifying exit directions...

Copy link
Member

Choose a reason for hiding this comment

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

I don't think special exits should be case-sensitive - most games aren't, it does not help for the mapper to be case-sensitive and also require all-caps cardinal directions.

Copy link
Member Author

Choose a reason for hiding this comment

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

You say most games aren't but we have to cover all games and because (up to the present and will be unless it gets changed¹) the text for a special exit represents the exact text that has to be sent to the MUD Server to use that Special exit we have to reflect that sensitivity in distinguishing exit identifiers.

Parts of the mapper (or rather the Rooms therein) ARE case sensitive - which is down to the (borked IMO) decision to just use strings as keys to the QMap/QHashs that hold details about custom exit lines, door markers and exit weights. The first of these uses CAPITALS as normal exit directions, the others used lower case. Of course - all of these ALSO support special exits as well (which is why text is used) - whereas other map features such as exit locks, and stub exits use numeric codes but can thus only support normal exits and does not work for special exits! That is why special exit locks are provided by the unholy mess that puts a '0' or '1' as a prefix onto the special exit text when it is stored as the value in the (QMap<int, QString>) TRoom::other member - which makes searching/iterating through special exits such a PITA.

This mess is how it is right now.

I want (and have played with code) to change to use QPair<quint8, QString> as a uniform exit identifier - with the first being the existing exit codes (1-12, 13 for Special) and second being QString() for the normal exit directions or the text for the special case. In some situations it ought to be allowable for some parts of the relevant code to allow first to carry the translated text for the normal exit direction but that would have to be worked out carefully...

1 - Long-term I have a plan to allow a short identifier, i.e. a name to be assigned to a Special exit which will be used for display purposes and then move the current text to be the command - with the default when importing old maps to set them both to be the current text - so that, for example, a current unlock Grate; open Grate; climb down; lock Grate could be given a name grate which would be much easier to use to identify that exit. Whilst this partially looks like an alias it would be carried in the Map data, specifically in the Room details. FTR The methodology is similar to how TinTin++ does it - although that client does this name/command separation for all exits - it is just that the normal directions have a default name-command mapping of 'n' to 'n', 'ne' to 'ne' etc.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, my point here is - should not be forced to give normal cardinal directions in all caps. Those should be case-insensitive. I don't agree on sensitive special exits either, but we can let it be.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I see what you are saying but we ideally we can't be case-sensitive for just the Special exit directions on (just those two commands) because we won't be able to tell in current map save files whether, for a custom exit line an "e" is a Special exit or a Normal one - OTOH this is broken corner case territory anyhow as for exit weights the same applies (in the opposite direction) for "E"... 🤷‍♂️

Given that it is already borked for this PR I'll propose making map format 20 and later:

  • handle both of these cases in a case insensitive manner.
  • convert the custom exit line keys (and the internal code) to be lower case for the keys in the custom line data held in the TRoom class (and remove the case-sensitivity for both setExitWeight and addCustomLine exit direction as a string).
  • revise the Wiki to note the change and (if it is not already there but I think it is) the existing ambiguity between Normal and Special exit directions.

Then - for a separate PR - revise the TRoom data structures to be more agnostic about whether the exit is Special or not. This would have the benefits, if done right, of:

  • allowing Special stub exits - i.e. for noting where there is such an exit but where the destination is not known during mapping.
  • probably allowing simpler C++ code when it is necessary to examine all of a room's exits.
  • it will also allow the exit lock data for Special exits to be handled in the same manner and simplify code in that area - and finally allow me to fixup or even eliminate TRoom::other.

After a review of existing lua commands that handle exit directions it may be desirable to declare some of them deprecated (but still supported, obviously) and provide replacements that have a uniform means of specifying the exit direction {in the manner I have previously mentioned, as a number(1-12) + nil (or rather ignored) for normal exits and 13 + string for special ones}...

…t dir

In fact have converted the TRoom custom exit line data members use a lower
case key for "Normal" exit directions and go through all the places where
those keys were used. Also make the change in the binary map data format
20 (and above). As it was convenient to do so, I have also changed it so
that the custom line colour is stored as a `QColor` instead of a
`QList<int>` of RGB components.

The addCustomLine and setExitWeight lua functions now treat a wider range
of strings as being for "Normal" exit directions in a case insensitive
manner - there is a small possibility that this may clash with strings used
for "Special" exits if they use a full English word for the exit direction
(with or without hyphens for diagonal exits)...

In testing the lua addCustomLine command I found that adding a new line to
a room that did not previously have one produced a weirdly visible line
that only showed up at some zoom levels and was very thin.  It turned out
that this is because there was not a call to TRoom::calcRoomDimensions()
after adding the line and the TRoom::{min|max}_{x|y} members (which are
updated by that) are used during the T2DMap::paintEvent().

Whilst I was working in this area of code I decided I could also provide
the missing lua function `removeCustomLine`...

Clang-format has been run over the areas edited - at least in this commit.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven requested a review from a team as a code owner November 14, 2018 21:58
Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Member Author

SlySven commented Nov 15, 2018

@vadi2 Has the recent commits improved things as far as you are concerned?

@vadi2
Copy link
Member

vadi2 commented Nov 15, 2018

Have not had a chance to look yet - this'll be a busy week.

Copy link
Member

@keneanung keneanung left a comment

Choose a reason for hiding this comment

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

Looks fine for me in general, though this is pretty huge and I might have overlooked something. There's one thing I noticed, though.

@@ -248,58 +250,88 @@ int TLuaInterpreter::Wait(lua_State* L)
return 0;
}

// Documentation: ? - public function missing documentation in wiki
// Documentation: PRIVATE function with no documentation in wiki.
// dirToString wiil now catch and validate pretty much any string that could
Copy link
Member

Choose a reason for hiding this comment

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

Here's a typo. Also the "now" qualifier is not useful here as we don't want to document past behaviour:

Suggested change
// dirToString wiil now catch and validate pretty much any string that could
// dirToString will catch and validate pretty much any string that could

Copy link
Member Author

Choose a reason for hiding this comment

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

🙁 I think we will have to because it now means the functions respond to more possible exit designations as normal exits - which may hit some corner case Special exits in some hopefully obscure circumstances (e.g. if there was a Special exit called, for example "East" in the past it would be treated as a Special exit but now the custom exit line is regarded as a Normal exit in that direction).

src/TLuaInterpreter.cpp Outdated Show resolved Hide resolved
@SlySven
Copy link
Member Author

SlySven commented Nov 16, 2018

Note to self: fix that using northeast instead of northwest typo... done.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven merged commit dfce0f0 into Mudlet:development Nov 17, 2018
@SlySven SlySven deleted the Enhance_improveCodeForRoomCustomExitLinesIn2DMap branch November 17, 2018 20:46
@@ -4652,48 +4661,46 @@ void T2DMap::slot_setCustomLine2()
if (!room) {
return;
}
if (exit == "NW") {
if (exit == QLatin1String("nw")) {
Copy link
Member Author

Choose a reason for hiding this comment

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

🪲 Unfortunately this code has subsequently proved to be buggy because exit is the text used on the clicked button in the form - and even the en_US case is broken because that used capital letters and not the lower case that this PR switches everything else over to...

SlySven added a commit to SlySven/Mudlet that referenced this pull request Feb 2, 2019
With the introduction of translations AND also after the conversion of the
internal exit direction keys for normal exits in the `TRoom` data
structures for custom exit lines, in:
Mudlet#2106
(which was included in Mudlet 3.16.0) it has not possible to create new
custom exit lines without encountering a bug that means the lines are
drawn in black and do not show up whilst drawing, the properties dialogue
also has no idea of the destination room and there are other oddities.

This commit contains an extract from:
Mudlet#1810
that was pending and had the fix for this problem

Note that this does not fix the lua getCustomLines(...) function which,
since 3.16.0 has also been using lower case keys to return the normal exit
direction custom exit line details.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit to SlySven/Mudlet that referenced this pull request May 23, 2019
…an 20

This will close Mudlet#2558 .

The bug was made in Mudlet#2106 which has
been present since before Mudlet 2.16.0 was released·

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request May 29, 2019
…an 20

This will close #2558 .

The bug was made in #2106 which has
been present since before Mudlet 2.16.0 was released·

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit to SlySven/Mudlet that referenced this pull request Aug 16, 2019
…ault

This PR removes support for saving in the binary map file format of Mudlet
2.1 - i.e. format **16** (retaining the ability to read it) and adjusts the
format for new map files to be **20** by default - up from the previous
default of **18**. This should speed up map loading a little - as it will
remove the need to convert the data to the current forms used internally as
well as making some details more robust. It also allows the removal of some
warning code that would have fired if area or map user data features were
used and format *16* was selected for saving.

#### History of recent (last six years) changes to Map File Format:

* dfce0f0 (PR Mudlet#2106) - **20** Improved way
that custom exit line data was held internally (so that the keys are now
the same as the other exit details that are keyed by a string {doors, exit
weight}. The custom exit line style is stored as the shorter and easier to
code with `Qt::PenStyle enum`  instead of a English `QString` and the
custom exit line as a `QColor` instead of a `QList<int>` with 3 elements -
(so the custom exit line could have a alpha component in the future!) Code
is in place to support a workaround to work within map formats back to
include version 17.

* 91a08c3 (PR Mudlet#1543) - **19** Added support
for more than one of any grapheme for the 2D map room symbol. Code is
in place to support a workaround to work within map formats back to include
version 17.

* 94dd41b (associated with PR Mudlet#301) - **18**
Added support for multiple user rooms in map file copied to other profiles
- so that the original player room (in the other profile) is retained in a
map copied over. Also revised the `TArea::rooms` from being a `QList` to a
faster to look up in `QSet`. Code in place to support saving in previous
formats.

* fb79c62 (associated with PR Mudlet#280) - **17**
Added support for area and map user data features. Warnings are issued
should these be actually used and a lower map format is specified to save
the map in **as that data will then be lost from the map file**.

* 88ef649 - **16** Map format of Mudlet 2.1
dating back to 2013-01-02 .

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added a commit that referenced this pull request Aug 21, 2019
…ault

This PR removes support for saving in the binary map file format of Mudlet
2.1 - i.e. format **16** (retaining the ability to read it) and adjusts the
format for new map files to be **20** by default - up from the previous
default of **18**. This should speed up map loading a little - as it will
remove the need to convert the data to the current forms used internally as
well as making some details more robust. It also allows the removal of some
warning code that would have fired if area or map user data features were
used and format *16* was selected for saving.

#### History of recent (last six years) changes to Map File Format:

* dfce0f0 (PR #2106) - **20** Improved way
that custom exit line data was held internally (so that the keys are now
the same as the other exit details that are keyed by a string {doors, exit
weight}. The custom exit line style is stored as the shorter and easier to
code with `Qt::PenStyle enum`  instead of a English `QString` and the
custom exit line as a `QColor` instead of a `QList<int>` with 3 elements -
(so the custom exit line could have a alpha component in the future!) Code
is in place to support a workaround to work within map formats back to
include version 17.

* 91a08c3 (PR #1543) - **19** Added support
for more than one of any grapheme for the 2D map room symbol. Code is
in place to support a workaround to work within map formats back to include
version 17.

* 94dd41b (associated with PR #301) - **18**
Added support for multiple user rooms in map file copied to other profiles
- so that the original player room (in the other profile) is retained in a
map copied over. Also revised the `TArea::rooms` from being a `QList` to a
faster to look up in `QSet`. Code in place to support saving in previous
formats.

* fb79c62 (associated with PR #280) - **17**
Added support for area and map user data features. Warnings are issued
should these be actually used and a lower map format is specified to save
the map in **as that data will then be lost from the map file**.

* 88ef649 - **16** Map format of Mudlet 2.1
dating back to 2013-01-02 .

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
vadi2 pushed a commit to vadi2/Mudlet that referenced this pull request Aug 21, 2019
* Refactor: extract a simple function that is used in several five places

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

* Refactor: eliminate shadowed variables in TTextEdit::mouseMoveEvent(...)

The use of `x` and `y` in nested code is confusing as the inner ones mask
the outer ones. This commit renames the inner copies.

It also makes a local reference to a section of the text attribute buffer:
`(std::deque<std::deque<TChar>>) TBuffere::buffer) that is used several
times.

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

* BugFix: cure the faulty selection when crossing an empty line.

The use of *manhattenLength()* was incorrect when determining whether it
was the start or end of the selection that was nearest to the mouse cursor,
instead it now (correctly) only considers the horizontal (x) offset when
the vertical offsets of the start and end point of the selection are equal.

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

* BugFix: finally cures the selection of text both with and without timestamps

Also:
* abstract the number of spaces for the timestamp on the left to a constant
`(int) TTextEdit::mTimeStampWidth`.

* renames `y` and `x` variables in the `TTextEdit::mouseMoveEvent(...)` to
`lineIndex` and `tCharIndex` as that more accurately describes them - and
also rename another pair of shadowing/masking `y` and `x` variables in the
SAME method to `yIndex` and `xIndex` respectively.

* convert a few remaining random C-style `(int)` casts in `TTextEdit` class
to C++ `static_cast<int>(...)` ones.

NOTE: An issue with deselection something in an upward direction against
the left margin remains - however the area is being correctly deselected
but code elsewhere that analyses which parts of the display actually
require repainting is missing that portion of the modified area of text.
It appears to be within the `TTextEdit::highlight()` method but it IS a
separate issue as the underlying text attribute buffer is being
correctly changed to clear the selection flags.

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

* Revise: make time stamps show in lower pane as well as upper one

It is visually disturbing for the lower pane in the main, error and debug
consoles not to match the upper one when it is revealed by scrolling - or
by turning on the timestamps for the main console.

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

* Revise: reduce space for time stamps from 15 to 13 (12 + space) characters

The use of 15 rather than 13 produced the need for an additional tweak when
trying to work out where the first actual game text start when trying to
deduce the position of it relative to the cursor!

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

* Fix insertPopup and echoPopup to accept "main" as an argument (Mudlet#3013)

* Update: drop support for saving to Mudlet 2.1 map format & update default

This PR removes support for saving in the binary map file format of Mudlet
2.1 - i.e. format **16** (retaining the ability to read it) and adjusts the
format for new map files to be **20** by default - up from the previous
default of **18**. This should speed up map loading a little - as it will
remove the need to convert the data to the current forms used internally as
well as making some details more robust. It also allows the removal of some
warning code that would have fired if area or map user data features were
used and format *16* was selected for saving.

#### History of recent (last six years) changes to Map File Format:

* dfce0f0 (PR Mudlet#2106) - **20** Improved way
that custom exit line data was held internally (so that the keys are now
the same as the other exit details that are keyed by a string {doors, exit
weight}. The custom exit line style is stored as the shorter and easier to
code with `Qt::PenStyle enum`  instead of a English `QString` and the
custom exit line as a `QColor` instead of a `QList<int>` with 3 elements -
(so the custom exit line could have a alpha component in the future!) Code
is in place to support a workaround to work within map formats back to
include version 17.

* 91a08c3 (PR Mudlet#1543) - **19** Added support
for more than one of any grapheme for the 2D map room symbol. Code is
in place to support a workaround to work within map formats back to include
version 17.

* 94dd41b (associated with PR Mudlet#301) - **18**
Added support for multiple user rooms in map file copied to other profiles
- so that the original player room (in the other profile) is retained in a
map copied over. Also revised the `TArea::rooms` from being a `QList` to a
faster to look up in `QSet`. Code in place to support saving in previous
formats.

* fb79c62 (associated with PR Mudlet#280) - **17**
Added support for area and map user data features. Warnings are issued
should these be actually used and a lower map format is specified to save
the map in **as that data will then be lost from the map file**.

* 88ef649 - **16** Map format of Mudlet 2.1
dating back to 2013-01-02 .

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

* BugFix: handle empty SGR...m sequences so they are treated as SGR0m

The empty (no parameter) case is the same as the one with a single 0 which
is the reset attributes case. Mudlet was not handling it as that but it
will do after this commit.

This will close issue Mudlet#2993 .

Also clean up some tabs used for spaces in the same method:
`(void) TBuffer::translateToPlainText(...)`

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

* Revise: some commented material in TTextExit::convertMouseXToBufferX

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
dicene pushed a commit to dicene/Mudlet that referenced this pull request Feb 19, 2020
…ault

This PR removes support for saving in the binary map file format of Mudlet
2.1 - i.e. format **16** (retaining the ability to read it) and adjusts the
format for new map files to be **20** by default - up from the previous
default of **18**. This should speed up map loading a little - as it will
remove the need to convert the data to the current forms used internally as
well as making some details more robust. It also allows the removal of some
warning code that would have fired if area or map user data features were
used and format *16* was selected for saving.

#### History of recent (last six years) changes to Map File Format:

* dfce0f0 (PR Mudlet#2106) - **20** Improved way
that custom exit line data was held internally (so that the keys are now
the same as the other exit details that are keyed by a string {doors, exit
weight}. The custom exit line style is stored as the shorter and easier to
code with `Qt::PenStyle enum`  instead of a English `QString` and the
custom exit line as a `QColor` instead of a `QList<int>` with 3 elements -
(so the custom exit line could have a alpha component in the future!) Code
is in place to support a workaround to work within map formats back to
include version 17.

* 91a08c3 (PR Mudlet#1543) - **19** Added support
for more than one of any grapheme for the 2D map room symbol. Code is
in place to support a workaround to work within map formats back to include
version 17.

* 94dd41b (associated with PR Mudlet#301) - **18**
Added support for multiple user rooms in map file copied to other profiles
- so that the original player room (in the other profile) is retained in a
map copied over. Also revised the `TArea::rooms` from being a `QList` to a
faster to look up in `QSet`. Code in place to support saving in previous
formats.

* fb79c62 (associated with PR Mudlet#280) - **17**
Added support for area and map user data features. Warnings are issued
should these be actually used and a lower map format is specified to save
the map in **as that data will then be lost from the map file**.

* 88ef649 - **16** Map format of Mudlet 2.1
dating back to 2013-01-02 .

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Translation of line design names
4 participants