Skip to content

Enhancement: move TArea::rooms to QSet (updated)#301

Merged
SlySven merged 20 commits intoMudlet:developmentfrom
SlySven:Enhancement_moveTAreaRoomsToQSet_updated
May 4, 2016
Merged

Enhancement: move TArea::rooms to QSet (updated)#301
SlySven merged 20 commits intoMudlet:developmentfrom
SlySven:Enhancement_moveTAreaRoomsToQSet_updated

Conversation

@SlySven
Copy link
Copy Markdown
Member

@SlySven SlySven commented Mar 22, 2016

A bit of a big one here but it does do something for bug 1500927 as mentioned in the forums. It also has a thorough map auditing/cleaner which should help to fix the sort of issues that past TArea::rooms addition/removal code introduced (particularly the removal part! 😄 ) and the custom exit line drawing code that would start to draw lines on an anonymous exit direction. E.g. this.

As I had to touch up the mapper drawing code the 2D one in particular has gained a bit of a tidy up for the room list selection widget - and it should be a bit faster because of the QList to QSet changes. Ssaliss' Aetherspace map (that +280MB map with a >2M room count, from "Lusternia" ? ) does load for me now (it does need at least 4GB of memory I think) and it works - slowly - which is better than before. The code also displays room names if there are any and the widget allows sorted on room number (or room names if they exist). The room selection code is also a bit more user friendly as the centre room (that single room operations on multi room selections and the spacing altering ones are centred on) is shown. Code to reposition the map on a new (singly selected, non-current) room is in-place (and sends a _sysManualLocationSetEvent_ that lua scripts can detect if they want!)

Users who use the same map on multiple profiles (possibly even at the same time - i.e. multiplying) may like the changes I have just made in that area which (provided the latest map file format 18 is selected on the last tab of the profile preferences) means that the map stores the current player location on a per profile basis rather than just as a single value - and gets the most upto date values for that from the profiles to which a map file is "shared" (well duplicated and copied). That has meant a revision to the map copying code which now needs to be told all the destination profiles to which it is to be sent and does them all in a single operation (and uses slots & signals to tell the active ones to reload the new map).

I have further code that hangs off of this that will improve the XML map file handling (will allow local loading of XML files that cannot be currently be done - much to the chagrin of some users).

I intend to:

  • get that working on development and then intend to port both across to release_30.
  • then fix up some of the zip file handling that may intersect with this and is a show-hinderer/stopper at the moment bug 1413069 I think it may also intersect with module handling!
  • do a quick & dirty fix on buttons missing command up & down command entries - but the button code does need a much bigger clean up as packaging of toolbars is broken IMHO - but that will need some XML handling changes (revision to Mudlet **non-**map file format sadly).

SlySven added 7 commits March 21, 2016 12:13
In situations when we check whether a room in an area both for internal
purposes and when rooms on a mapper is selected, using QSet instead of a
QList is faster in performance for large numbers of entries in set.

As it reworks the mapper code it also fixes issue where the multi-room
selection widget overwrites map info display - the latter is re-sized and
re-positioned (and regains a semi-transparent background which helped to
show this working during debugging!)  The former is now: dynamically
resized to only take up enough vertical space to show the selected rooms;
also displays the associate room names if there are any, expanding the
widget as required; sorts the display either by room name or number and in
either direction.

The mouse wheel handler is modified so that using the scroll wheel ONLY
scrolls the list within the widget - previously (by default) once the end
in either direction was hit the related events would be passed up the
widget chain where it would otherwise invoke the 2D mapper's zoom in/out
code.

In modifying the zoom in/out code I have replaced the (obsoleted in Qt5.x)
QWheelEvent::delta() method to use the QWheelEvent::angleDelta() method,
using only the Y-component the latter provides.  If the Control modifier is
active the zoom value is modified by an extra x10 factor which is useful
when working with large maps as otherwise the zooming rate is "slow" at
high values - ideally the control should be logarithmic or exponential or
some other "non-linear" algorithm to work more uniformly over the range of
practical use cases.

The code to paint the map info text has been revised also to use the
mMapInfoRect which was being defined but NOT used.

The info text now reports whether the room name is for the player room
{set via the Lua command centerview(roomId)} or is one that is selected
by mouse dragging - and if more than one room is selected by that indicates
the count of rooms in the selection.  In the case of multiple rooms being
selected the room that single room context menu operations will act upon
is highlighted by the same style of yellow target used to show the custom
exit line destination but is drawn in a different point in the code so that
it is drawn over the rooms.

Because of the change to the way that multiple rooms are selected routines
that use that information had to be revised - in doing so it was possible
to improve the usability/operation of:
T2DMap::slot_movePosition()
T2DMap::slot_setCharacter()
T2DMap::slot_spread()
T2DMap::slot_shrink()
T2DMap::slot_lockRoom():
T2DMap::slot_unlockRoom():
This method, also resurrected here to the 2D mapper context menu, as it is
also affected by the changes:
T2DMap::slot_setPlayerLocation()

There was a slot_setPlayerLocation code that set a global lua variable
mRoomSet and moved the player to that room Id (introduced in
commit-c25faf4e 2012-05-04 07:44:36 by Heiko) but the corresponding 2D
Mapper context menu item that called it was commented out and thus removed
from the menu in commit-93f65962 2012-12-29 01:16:28 also by Heiko without
any explaination. Since that has not been used since then I have replaced
it with a new Event: sysManualLocationSetEvent with a single numeric value
which is the new (valid) room Id number - user scripts can capture this
event if they want to know that the user has manually re-positioned the
current player room in the 2D mapper.

In passing:
* Fixed Text font changing between docked and un-docked forms of the
built-in map widget (when not incorporated into a console) - as it was
not previously explicitly set it assumed the Application one whilst
docked but the Qt System one when a free floating widget - and the two
do not have to be the same. This fixes:
https://bugs.launchpad.net/mudlet/+bug/1432841 .

* Starts to fix https://bugs.launchpad.net/mudlet/+bug/1376511 by changing
from use of obsolete QWheelEvent::delta() to QWheelEvent::angleDelta() in
T2DMap::wheelEvent(...); will need duplicating in
TTextEdit::wheelEvent(...)  and GLWidget::wheelEvent(...) .

* Adds the profile name to the Mapper dockable widget so that it's
parentage can be determined when multiple profiles are active.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Found during testing with humongous map files that the "Loading" & "Saving"
messages did not show up - turns out a qApp->processingEvents() call is
needed...  Whilst fixing that in relevant places on that the Profile
Preferences dialog also applied QStringLiteral(...)/ tr(...) wrappers as
needed.

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

It is possible to copy a saved map from one profile to another - even if
that other profile is in use.  However this will lose the details of where
the (other) player is located in THAT profile.  This commit aims to correct
that by sneaking a look at where the other player is and storing that
in the file that is both copied for this profile before it is copied and to
the file that is copied over.

This data was stored in (int) TMap::mRoomId which has now become a:
(QHash<QString, int>) TMap::mRoomIdHash with the profile name as the key
and the Value being the equivalent of mRoomId in the new code.  This has
required an increment in the Map file format version from 17 to 18.  The
default - that will be used unless the developer or tester changes it in
the Profile Preferences dialog remains at 16.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
As well as fixing the Area ownership of rooms (which has been an issue that
crops up from past buggy TArea code) this also clears out custom exit line
junk - including details for lines where the key was an empty string, those
arose during previous buggy code that allow exit lines to be drawn without
specifying which exit they were for.

The checks also removes any stub exits that are present on rooms that have
an actual exit in the same (normal exit) direction.  The current code
prevents the creation of such things but it did not in the past (I have
encountered it - especially when the real exit is a "circular" one that
goes to the SAME room as it leaves from!)  BTW Such a exit type, although
initially seeming to be pointless is a feature of some MUDs and the normal
exit drawing code for the 2D map does not currently show them (future
thing to cover?) though a custom exit line can be drawn that shows the
"circular-ness" of the exit!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Refactor the code that identifies where player is in other profiles, saves
it in current profile's map, copies it to other profiles and tells them
to load it in.

The code to copy to another map as was only permitted one other profile to
be done at a time - this makes it difficult to ensure that all the profiles
get the same data for all the other affected maps. The profile selection
part of "Profile Preferences" "Mapper" has been changed to be something
that allows all the destination targets (other profiles) to be selected,
excluding the current profile (as that ALWAYS gets the new map - it is
intrinsic to the copying process).  Only when one (or more) other profiles
are selected does the "Copy" button become enabled and then all active
profiles are checked to see if they are included, their player room is
noted as before and all the details are written to the current profile's
new map.  After copying (with some checks for success) the map to ALL
profiles a signal is sent to all active profiles (with a list of profiles
to respond) that causes the listed ones to reload a new map from their
home map directory.  (Extensive, formatted!) tooltips have been added to
the two controls concerned in the profile preferences.

Slots and signals are used (via the singleton class mudlet) to try and
accommodate future situations when Mudlet becomes properly multi-threaded
and each profile runs in a different thread - getting the current room
from each profile may become more tricky THEN.

Also a bug where the currently selected (in the same dialog) map save file
format was not being used when the current profile's map has been changed
has been fixed in this commit.

===========================================================================
To support the inclusion of other profile player location data in the map
file  MAP FILE FORMAT 18 (or greater) MUST BE CHOOSEN BEFORE THE MAP IS
COPIED.
===========================================================================

If this is not done then the map will not contain the data and then the
other profiles' player will be reset to the location of the player in the
currently active profile - as would happen for the old code (prior to
commit entitled "Enhance: store current player location in map file on a
per profile basis").  One bonus of this code is that only one copy of the
map to be copied over is made in the current profile's directory and all
the copies will have the same date-time stamp name between the profiles,
instead of a sequence of differently timed files as the file is copied one
profile at a time that code prior to this series of commits will produce!

Some items in the profile_preferences.ui have been renamed to clarify their
type or purpose or change thereof:

QGroupBox:
* groupBox_3               ==> groupBox_mapFiles

QLabel:
* label_9                  ==> label_saveMap
* label_10                 ==> label_loadMap
* label_12                 ==> label_copyMap
* map_file_action          ==> label_mapFileActionResult

QPushButtons:
* copy_map_profile         ==> pushButton_copyMap
* load_map_button          ==> pushButton_loadMap
* save_map_button          ==> pushButton_saveMap

Changes:
* (QComboBox) mapper_profiles_combobox
                           ==> (QPushButton) pushButton_chooseProfiles

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
I forgot to replace a #define constant with a variable when a literal block
of code was moved to a function.  The effect means that some map auditing
code will report some faulty exit details as being related to a north
exit whatever the direction involved was!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Mar 23, 2016

Build issues on all Linux platforms looks like a Qt library issue for QTreeWidget probably need a QT_VERSION wrapper around a fix.

C.I. Testing detected this issue around QListWidget::setSizeAdjustPolicy
which is only available on Qt5.2 or later.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 25, 2016

Looks OK to me. I don't get the "loading map" message from 06cbeeb, but it doesn't matter that much because the filesystem dialog is still open while the whole client is frozen.

+1

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Mar 25, 2016

Vadim wrote:

I don't get the "loading map" message from 06cbeeb, but it doesn't matter that much because the filesystem dialog is still open while the whole client is frozen.

I think we will have issues going forward with Q(File)Dialog at least, according to the QFileDialog Documentation:

On Windows and OS X, this static function will use the native file dialog and not a QFileDialog. However, the native Windows file dialog does not support displaying files in the directory chooser. You need to pass DontUseNativeDialog to display files using a QFileDialog. On Windows CE, if the device has no native file dialog, a QFileDialog will be used.

On Unix/X11, the normal behavior of the file dialog is to resolve and follow symlinks. For example, if /usr/tmp is a symlink to /var/tmp, the file dialog will change to /var/tmp after entering /usr/tmp. If options includes DontResolveSymlinks, the file dialog will treat symlinks as regular directories.

On Windows, the dialog will spin a blocking modal event loop that will not dispatch any QTimers, and if parent is not 0 then it will position the dialog just below the parent's title bar.

I guess we will have to look at using QFileDialog::DontUseNativeDialog ...! 😄

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 25, 2016

Let's definitely use the native dialog, non-native dialogs look awful on the users system and are harder to use (because people aren't use to them). That tradeoff definitely isn't worth just so you can see the message that it's loading, when the client is frozen anyway...

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Mar 25, 2016

That is just it - the client is FROZEN, is that always going to be acceptable? If the user is multi-playing - for example? I suppose such hanging is irritating during session start-up, connection setup and tear-down and session ending but less so if another connection has to be maintained. 😟

Though we don't explicitly use threads they are implicitly used under the hood and, ideally, each profile ought to be a separate thread (with the mudlet class running in the master, supervisory & GUI running thread role I guess)? The Qt Docs says:

More interesting is that QObjects can be used in multiple threads, emit signals that invoke slots in other threads, and post events to objects that "live" in other threads. This is possible because each thread is allowed to have its own event loop.

Can we get away with a warning that some actions are best done with all profiles being quiescent when multi-playing - like starting or ending a profile - it does actually make some sort of sense I suppose as you are not going to be wanting to interact with the "Connect" dialog whilst doing Hand-to-Hand PvP!

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Mar 25, 2016

It is never acceptable to have the client frozen, but I don't think we need a warning here either, because people don't manually load the map all that often, and certainly not in the middle of pvp.

I think this pull request is good to be merged as-is, I just noted that the fix for the loading message didn't work as expected but there's not that much need to have it be working.

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Mar 25, 2016

OK - I'll port this to the release_30 branch over the next day or so (so the map file formats stay in step) - before merging...

...um, OK I got held up with other stuff - I haven't forgotten this - honest 😊

Recent code changes uses longer messages in the "label_mapFileActionResult"
in the "Mapper" tab of the "Profile Preferences" dialog, it was however
only occupying one of the three columns in the QGridLayout in the part of
the dialogue where it was place - so that when a long message is displayed
it caused all the surrounding widgets in the other columns in the same
layout to be pushed out of the way.  This tweak makes it occupy ALL three
columns so this no longer occurs!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
SlySven added 2 commits April 18, 2016 01:20
One build version on Travis C.I. (Linux/Cmake/GCC) for a different code
branch did not seem to like the use of nullptr - so I have reverted to the
ubiquitous numeric 0 instead.  This is because it is a C++11 feature and
there is no requirement to use C++11 specified in the CMake project file.

In passing also noted that a few used files ./src/irc/ircglobal.h,
./src/irc/IRCGlobal & ./src/irc/IrcSender are used but not listed in the
qmake ./src/src.pro project file - so they do not show up in the Qt Creator
IDE - and the first contains the version number of the Communi IRC library
that we incorporate and use.  So, I've added it to the project file as
well.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Although we have the provision of a default area (with Id -1) we have not
previously actually made sure it exists after loading (or clearing) prior
to loading in a map - which causes all sorts of error messages when a map
WITHOUT that area is loaded.

This will possible need checking with a planned revision to code to allow
user loading of XML map files in the near future.

Also:
* moved all instance in TMap class of mpHost->postMessage( QString ) to own
  postMessage( QString ) implimentation
* removed unused QDataStream reference argument in:
  * (void) TRoomDB::restoreSingleArea( QDataStream &, int, TArea * )
  * (void) TRoomDB::restoreSingleRoom( QDataStream &, int, TRoom * )
* removed unused Globals in XMLimport.cpp:
  * int maxAreas
  * QMap<int,int> areaMap

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Apr 21, 2016

Humm, seems to have a bug which is causing doors to be removed from normal direction exits even though there is an exit or a stub (will be checking to see if it is related to the code that prevents a stub being on the same exit direction as a real exit, when in that case the stub is automatically removed...)

#308 will have the same bug.

SlySven added 2 commits April 22, 2016 02:34
Also refactored TLuaInterpreter::setDoor(...) & getDoor(...) to current
error handling standards - including checks for validity. The name of the
normal exit directions always were specific but are now explicitly checked
for - this goes in step with the recently added auditing code that removes
doors on exits that do not exist (even as a normal direction "stub" exit).

The setDoor(...) function returns true if a change was made to the room's
doors and false if the command was ineffective.  It now also returns a
secondary error message if a nil is returned to explain what the problem
was...!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
Needed to move a line:
  exitStubsPool.remove( dirCode );
outside of an:
  if( ! exitStubs.contains( dirCode ) ) {...}
test in (void)TRoom::auditExit(...) so that an extra warning message will
may be avoided in a potential corner-case - though I'm not sure I can
express when it might occur!

Also:
* Extend info/warnings about more elements of an invalid exit Id; in
  reviewing what is preserved in the room user data in the event of
  encountering an invalid exit id (<1 but not the "no exit" -1 case) we can
  preserve information about whether the exit was locked and had an exit
  weight easily (but not a custom exit line that would be much harder) in
  the room user data - with information to the user about all three.
* prevent use of TRoom::setExitLock( int, bool ) from causing duplicates
  in the (QList<int>) TRoom::exitLocks member - otherwise it caused
  that correctable issue to occur which adds to the volume of audit
  messages.
* Fix a recently introduce bug in the Lua setDoor(...) command that would
  fail to set a door on a specialExit as the code to validate the command
  forgot the lock indication prefix of '0' or '1' in the exit command.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
I had got the logic wrong in identifying whether an exit was a normal or
a special exit - so the validation did not work correctly as to whether a
door could be allowed to be set/reset in a room.

Also: added a call to refresh the 2D mapper display if a change WAS
successfully made to the doors for a room...

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Apr 29, 2016

I think this and the release_30 PR are now good to go - just wanting a 👍 from @vadi2.

Mind you I thought I said that a hour or two ago but it seems I must have left this page without posting the comment and lost it, weird! 😕

Anyhow I'm going back to try and make sense of the toolbar/menu/buttons - they need some ❤️ and possibly a bit of 🔧 or more likely a big 🔨!

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Apr 30, 2016

Can we have the audit information only be shown if you use loadMap or something with a flag set? I was greeted with this when I loaded the map, and nobody but the mapmaker is going to be able to do with that enormous amount of information.

src/TRoomDB.cpp Outdated
QSet<int> _set = pR->exitStubs.toSet();
if( _set.count() < _listCount ) {
QString infoMsg = tr( "[ INFO ] - Duplicate exit stub identifiers found in room Id: %1, this is an\n"
"anomoly but has been cleaned up easily." )
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.

Typo, should be "anomaly"

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.

An exit lock is a boolean status, so what does it mean by a duplicate exit stub identifier?

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Apr 30, 2016

"In room with Id: 2054 found one or more surplus custom line elements that were removed: ." what's a surplus custom line element and what does the empty string mean?

What's a surplus door item as well?

This detail was missing and was confusing when centerview(roomId) was used.

Also, handle the case when the displayed map IS the default area (through
using centerview(...) when the room is in that area AND the area widget has
been set to hide that area name OR it has just been told not to!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Apr 30, 2016

In area with Id: 1 there were 10 extra rooms compared to those it
            should be recording as possessing, they were:
            9720, 417, 416, 9717, 9716, 418, 9719, 14756, 9718, 14752
            they have been removed.

In area with Id: 2 there were 17 rooms missing from those it
            should be recording as possessing, they were:
            37430, 25389, 25391, 25392, 28069, 20107, 20106, 20089, 20088, 20090, 20085, 20084,
20087, 20086, 20083, 20082, 25299
            they have been added.

What do these messages mean?

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Apr 30, 2016

The mapper at a quick glance still works, and the area centering via the area dropdown works, so the critique is just on the cosmetic stuff. Great work.

@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented Apr 30, 2016

Vadim wote:

Can we have the audit information only be shown if you use loadMap or something with a flag set? I was greeted with this when I loaded the map, and nobody but the mapmaker is going to be able to do with that enormous amount of information.

This could be tricky - as the map could be loaded before the user has a chance to set or unset a flag (autologin cases or even starting a profile normally after an update to this version of Mudlet!) and map loading can be asynchronous to user actions (some time after the user has requested the map from the profile preferences or from initial connection to an I.R.E. MUD) - or now, triggered from a different profile which is sharing the map with the one concern whilst multi-playing (though in that case the map should be "clean" already).

"In room with Id: 2054 found one or more surplus custom line elements that were removed: ." what's a surplus custom line element and what does the empty string mean?

Back when we had that buggy code that allowed custom exit lines to be drawn before a direction was selected from the dialogue concerned (or the dialogue completely failed to be displayed and it went straight to drawing the line) those lines ended up being stored against an empty string key in the TRoom::customLine* members. Also, as the specifications for those custom exit lines are spread across several TRoom class members, the code to delete them did not necessarily clear out all entries against a specific key (which, of course for "normal" exit custom lines are the UPPERCASE "N","NE","E,","SE","S","SW","W","NW","UP","DOWN","IN","OUT" and for special exits are the text that has been stored {can be a complete lua script as it is!}). Those surplus elements are things that have been found that do not match up to actual exits possessed by the Room concerned - and the messages are warning that they have been purged.

What's a surplus door item as well?

Similarly a door without a normal exit (or stub - the code has to allow doors even if we do not know where the exit goes to - as this is a "real-life" situation when map-making) or special exit is being removed. Though we do not (yet) show doors on all of these yet this will come once I enhance the custom exit line and stub exit code to support door symbols (which will be a 2x exit line thickness "bar" across the exit line at a distance of 0.45 of the inter room distance {for rooms in adjacent positions but I'd increase that to a larger but fixed position where the distance is greater}). Anyhow this is a warning that these unused/surplus items have been purged.

In area with Id: 1 there were 10 extra rooms compared to those it should be recording as possessing, they were: 9720, 417, 416, 9717, 9716, 418, 9719, 14756, 9718, 14752 they have been removed.
In area with Id: 2 there were 17 rooms missing from those it should be recording as possessing, they were: 37430, 25389, 25391, 25392, 28069, 20107, 20106, 20089, 20088, 20090, 20085, 20084, 20087, 20086, 20083, 20082, 25299 they have been added.
What do these messages mean?

This is the fixup of the TArea::rooms where the past buggy code did not correctly move rooms from the default area (or any other) to a different area. By comparing the individual TRoom::area values to the the Areas' idea about which room they have these report the discrepancies that have been "fixed" - the first are rooms that the area thinks it has but in fact does not (they have been deleted or now belong to a different area - possibly the "Default" one) the second are rooms that the area does not realise that it has. This second group is likely the cause of "disappearing" rooms as the map drawing code uses the TArea::rooms as the rooms to draw for a particular area - and any rooms missing from that will not be shown.

As to hiding these details I not sure whether we ought to - they are one-shot and they are indicating (hopefully irrelevant) data loss; and once the corrected map has been saved most of the messages will not be seen again. If the lost data is NOT irrelevant at least the user can scroll back through the buffer and work out what is happening. If we do not show most of this data by default I suppose I could make use of the now unused error.txt file but I then have to start thinking which things are important enough to continue to show and which can be silently passed off or put into that file... do you have any guidance you want to offer on these?

@vadi2
Copy link
Copy Markdown
Member

vadi2 commented Apr 30, 2016

On Sat, Apr 30, 2016 at 4:41 PM Stephen Lyons notifications@github.com
wrote:

Vadim wote:

Can we have the audit information only be shown if you use loadMap or
something with a flag set? I was greeted with this when I loaded the map,
and nobody but the mapmaker is going to be able to do with that enormous
amount of information.

This could be tricky - as the map could be loaded before the user has a
chance to set or unset a flag (autologin cases or even starting a profile
normally after an update to this version of Mudlet!) and map loading can be
asynchronous to user actions (some time after the user has requested the
map from the profile preferences or from initial connection to an I.R.E.
MUD) - or now, triggered from a different profile which is sharing the map
with the one concern whilst multi-playing (though in that case the map
should be "clean" already).

OK, need to think on this then, as we definitely cannot have the current
wall of text default. Most people aren't mappers, the information will be
of no use to them.

What if we write it to the debug log, and if there were issues detected,
say "See debug log for more information."?

"In room with Id: 2054 found one or more surplus custom line elements that
were removed: ." what's a surplus custom line element and what does the
empty string mean?

Back when we had that buggy code that allowed custom exit lines to be
drawn before a direction was selected from the dialogue concerned (or the
dialogue completely failed to be displayed and it went straight to drawing
the line) those lines ended up being stored against an empty string key in
the TRoom::customLine* members. Also, as the specifications for those
custom exit lines are spread across several TRoom class members, the code
to delete them did not necessarily clear out all entries against a specific
key (which, of course for "normal" exit custom lines are the UPPERCASE
"N","NE","E,","SE","S","SW","W","NW","UP","DOWN","IN","OUT" and for special
exits are the text that has been stored {can be a complete lua script as it
is!}). Those surplus elements are things that have been found that do not
match up to actual exits possessed by the Room concerned - and the messages
are warning that they have been purged.

This implies that the lines the user has drawn might have been deleted - is
that the case, just to be sure? That's not ok to do...

What's a surplus door item as well?

Similarly a door without a normal exit (or stub - the code has to allow
doors even if we do not know where the exit goes to - as this is a
"real-life" situation when map-making) or special exit is being removed.
Though we do not (yet) show doors on all of these yet this will come once I
enhance the custom exit line and stub exit code to support door symbols
(which will be a 2x exit line thickness "bar" across the exit line at a
distance of 0.45 of the inter room distance {for rooms in adjacent
positions but I'd increase that to a larger but fixed position where the
distance is greater}). Anyhow this is a warning that these unused/surplus
items have been purged.

Sounds OK since these weren't visible previously.

In area with Id: 1 there were 10 extra rooms compared to those it
should be recording as possessing, they were:
9720, 417, 416, 9717, 9716, 418, 9719, 14756, 9718, 14752
they have been removed.
In area with Id: 2 there were 17 rooms missing from those it
should be recording as possessing, they were:
37430, 25389, 25391, 25392, 28069, 20107, 20106, 20089, 20088, 20090,
20085, 20084,
20087, 20086, 20083, 20082, 25299
they have been added.
What do these messages mean?

This is the fixup of the TArea::rooms where the past buggy code did not
correctly move rooms from the default area (or any other) to a different
area. By comparing the individual TRoom::area values to the the Areas'
idea about which room they have these report the discrepancies that have
been "fixed" - the first are rooms that the area thinks it has but in fact
does not (they have been deleted or now belong to a different area -
possibly the "Default" one) the second are rooms that the area does not
realise that it has. This second group is likely the cause of
"disappearing" rooms as the map drawing code uses the TArea::rooms as the
rooms to draw for a particular area - and any rooms missing from that will
not be shown.

OK.

As to hiding these details I not sure whether we ought to - they are
one-shot and they are indicating (hopefully irrelevant) data loss; and
once the corrected map has been saved most of the messages will not be seen
again. If the lost data is NOT irrelevant at least the user can scroll back
through the buffer and work out what is happening. If we do not show most
of this data by default I suppose I could make use of the now unused
error.txt file but I then have to start thinking which things are
important enough to continue to show and which can be silently passed off
or put into that file... do you have any guidance you want to offer on
these?

See if my suggestion above will work with you.


You are receiving this because you were mentioned.

Reply to this email directly or view it on GitHub
#301 (comment)

As the amount of messages produced by the new/improve map auditing/clean-up
code can be large for old map files this commit causes them to be sent to
a profile specific file that is already opened by the Host class but has
not been used for some time.  An option (a check-box) on the "map" tab of
the "profile preferences" dialog controls whether the equivalent
information is also shown on the profile's main console as in the past.

If the option (a global one) which is saved between sessions/runs is NOT
enabled then an advisory is sent to the console advising the user to review
the file contents if a "significant" issue was detected.  If it is enabled
a similar message advising that the information has also been saved. In
either case, as the file is appended to, and not rewritten each time, the
message includes details of the first line of the report so that it can
be located in the file.  One difference between the on-screen and the file
versions of the information is that the former puts up the issues as they
are detected whereas the latter groups the information by subject, first
the general overall issues, then the area ones, in order of the areas' id
numbers and then the room ones, in room order.

Following earlier discussions in these Pull Request I have attempted to
edit all uses of the word "Id" in user visible places to be "id" instead.
This threw up some other messages with issues in several files that I have
tweaked here as well!

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven
Copy link
Copy Markdown
Member Author

SlySven commented May 3, 2016

Oh, b****r - I've messed up. Somehow this PR has diverged from the branch so that I can't plonk this series of commits into the latter cleanly. I will have to see if I can tweak it back into shape preferably without re-basing... the other branch is fine though!

Some recent changes caused the Pull Request on GitHub to become
unmergeable, the changes here should be sufficient to allow it to proceed.

Signed-off-by: Stephen Lyons <slysven@virginmedia.com>
@SlySven SlySven merged commit 4f6a41d into Mudlet:development May 4, 2016
SlySven added a commit that referenced this pull request May 4, 2016
…aRoomsToQSet_updated

(release 30)enhancement: move TArea::rooms to QSet (updated)

As per my merge comment for #301 am merging this.
@SlySven SlySven deleted the Enhancement_moveTAreaRoomsToQSet_updated branch March 26, 2017 21:19
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>
@SlySven SlySven restored the Enhancement_moveTAreaRoomsToQSet_updated branch June 22, 2020 18:04
@SlySven SlySven deleted the Enhancement_moveTAreaRoomsToQSet_updated branch June 22, 2020 18:23
mehulmathur16 pushed a commit to mehulmathur16/Mudlet that referenced this pull request Feb 16, 2024
And avoid a trailing newline for custom loggers.

fixes Mudlet#298
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.

2 participants