Any one have any objections to merging this?

Sorry, I have not reviewed this yet! I would like to when I can.

This is so verbose....can you simply say if it's a bug fix and if it
introduces anything incompatible?

Sent from my phone.
On Nov 10, 2014 6:59 PM, "Vadim Peretokin" notifications@github.com wrote:

Sorry, I have not reviewed this yet! I would like to when I can.

—
Reply to this email directly or view it on GitHub
#219 (comment).

Yes - it fixes the specific bug - but theoretically anyone relying on the data involved (which I don't think there can be) should run auditAreas() to fix it up on existing maps. I apologise if I seem to go on a bit in commit comments but that does seem to be the place to leave anything that a change-log or other reviewing process might need.

I have to agree, this information is useful to know.

On Wed, Nov 12, 2014 at 10:24 AM, Stephen Lyons notifications@github.com
wrote:

Yes - it fixes the specific bug - but theoretically anyone relying on the
data involved (which I don't think there can be) should run
auditAreas() to fix it up on existing maps. I apologise if I seem to go on
a bit in commit comments but that does seem to be the place to leave
anything that a change-log or other reviewing process might need.

—
Reply to this email directly or view it on GitHub
#219 (comment).

It is still weird is best I can describe it. At first I loaded the map (get it here) and only got this result for area 44:

{ 12477 }

Which is a valid area exit room, but there are more than that.

Then I did a pathfind, and the result changed to:

{ 7091, 11400, 11841, 11911, 11912, 11913, 11915, 11916, 11919, 11933, 12057, 12185, 12294, 12383, 12468, 12483, 12510, 13713, 14551, 14829, 15510, 15610, 15810, 24012 }

Which includes non-area exits like 11841.

The actual list is:

{ [12483] = "In a chamber of skulls", [11122] = "Pestilent Way", [11112] = "Nefarious Way", [24012] = "Before the Mhaldor Cathedral", [11400] = "The gates of Mhaldor", [11133] = "Abomination Road", [7091] = "Approaching the temple", [10659] = "Central courtyard" }

Which is generated by this Lua-side function:

function mmp.getAreaBorders(areaid)
    if mmp.debug then
        mmp.getAreaBordersTimer = mmp.getAreaBordersTimer or createStopWatch()
        startStopWatch(mmp.getAreaBordersTimer)
    end

    local roomlist, endresult = getAreaRooms(areaid), {}
    -- sometimes getAreaRooms can give us no result :(
    if not roomlist then mmp.echo("Sorry, seems we can't go there - getAreaRooms("..areaid..") didn't give us any results (Mudlet problem - redownloading the map might help fix it)") return end

    local getRoomExits, getRoomName, getSpecialExitsSwap, contains, pairs = getRoomExits, getRoomName, getSpecialExitsSwap, table.contains, pairs

    -- make a key-value list of room IDs
    local reverselist = {}
    for i = 0, #roomlist do reverselist[roomlist[i]] = true end

    -- obtain a room list for each of the room IDs we got
    --for _, id in pairs(roomlist) do
    for i = 0, #roomlist do
      local id = roomlist[i]
      local exits = getRoomExits(id)
      for _, to in pairs(exits) do
        if not reverselist[to] then
          endresult[id] = getRoomName(id)
        end
      end
      local specialexits = getSpecialExitsSwap(id)
      for _, to in pairs(specialexits) do
        if not reverselist[to] then
          endresult[id] = getRoomName(id)
        end
      end
    end

    if mmp.debug then
        mmp.echo("mmp.getAreaBordersTimer() on areaid "..areaid.." took "..stopStopWatch(mmp.getAreaBordersTimer).."s to run. Returned "..table.size(endresult).." results.")
    end

    return endresult
end

Also what does TRoom::auditExits() WARNING: roomID:11051076 REMOVING invalid (general) exit: <exit> exactly mean... ?

This doesn't fix getAreaExits() but does fix auditAreas(), so +1.

Since this is a broken function, let's make the output better. A table with exit rooms as keys and the room they go to as value?

Well since I've got a better idea about Lua tables now (thanks to that gitter hint) I'll tack something on, but I think directions will be the keys. Of course it can be the case that one exit room has multiple exits in different directions to either the same or different rooms.

Though since Mudlet doesn't have separate "name" and "command" items for Special Exits c.f. TinTin++ where an exit has something you type to use it - which for "normal" exits is just something like "e" or "ne" but could also be say what we have as a "special" exit "jump out" which is separate from the "command" that is sent to the MUD server - the key could become a long lua script...

One room can go to several different areas though, which complicates it. Just an indexed list of the room id's or a key-value list with the room id and room name will be enough.

For possible future work on chunked path-finding, a key-value pair of from->to is better. I implemented as such, though this pull is needed for it to work as well, along with a map version increase.

Can you add a version increase @SlySven and rename the functions something English? With the version 17 re-running the code to populate the exits. Then we can merge in #225 which needs this to operate.

From a room to a room? But a room can have several exits leading to other
areas.

Is there anything else that we want to bring in with a map file format increment?

• I've got some "file magic" I want to include {so that *nix systems at least can examine the file with the file(1) command and use some magic(5) to report some data about the file: the fact that it IS a mudlet map file and number of rooms, number of area and the UTC datetime it was written.} The use of the generic ".dat" extension just compounds the lack of metadata about such files available to things that are NOT the Mudlet application. 😜
• We can lose the useless (TArea *)->z{min|max}Ebenen members.
• With an overhaul to (TArea *)->koordinatenSystem() to use and retain its data, and convert the QMap<int, QMap<int, QMultiMap<int, int> > > structure where from the outside to in the keys are the x, y and z coordinates to a QMap<int, QMap<int, QMap<int, QSet > > > with the key order reversed I believe we can improve on the 2D and particularly the 3D mapper paint code - as well as establishing very quickly room spatial collision. Ah, that isn't tied to the file format, we could regenerate it on loading.

Nothing else, since this commit needs to go into 3.0 and 3.0 is frozen for new features - we're just trying to fix all the new issues introduced and get it released.

@SlySven Just a version # increase to 17 and rerun initAreasForOldMaps if version < 17 (look at the code in TMap::init)

Yeah, that initAreasForOldMaps() (run by TMap::init() for map file formats less than 15 IIRC) runs the same bit of code needed as the lua auditAreas() does - the downside of incrementing the map file version being that shared/crowd sourced maps then become unusable to anyone not using the beta at this point in time. Whereas implementing the code in this PR so far and advising users to use "lua auditAreas()" once on existing maps (and subsequently each time they use their map on the Mudlet application built on this or later code if it has been touched by an earlier Mudlet application instance) may be less disruptive at this point in the release process.

I'd prefer to combine the auto map data fixup with the next other change that forces a map version increment unless you really,really want it done with such an increment now Chris ❓

I have no idea how the crowd sourced maps work. How are they updated? If someone does the lua auditAreas(), then uploads that map, it would be the same problem, correct?

If you increment the version number, you increment the Mudlet version dependency. This means that if the person doing the mapping upgrades to a new Mudlet, nobody else on the old Mudlet can use the map. So ideally, this happens fewest times possible because it's a pita for everyone involved because of the abscence of forward compatibility.

Just run the map fix thing automatically, no need to actually change the map format until the 'magic numbers' come in, yeah? Which we can bundle with something else since very, very few people are actually going to be making use of that feature.

Sounds good @vadi2 . If they are adding a new feature, they can do the run. Here's another idea then as well -- how about a mudlet version/map version fetcher for lua so scripts can run any migration code that is needed. To make it easier we can even bundle it in LuaGlobal (or whatever) so there aren't 15 different migration scripts.

@SlySven Do you want to change the mapping to:
QMap<from_id, QPair<QString(exit), to_id> >

or you can merge this and I'll change that code with getAreaExits fix.

Do you mean QMap > or something else, you keep mentioning it and it seems something is getting cut off.

The issue is forward compatibility, old Mudlet can't load the a binary map from a newer Mudlet - not sure Lua scripts can do much here.

yeah, it's the markup.

Well we can't change the exits variable without a version increase, since the new map will be storing the information differently and importing the old map will throw an error.

We can address this by a new variable, or a version increase. I don't like having cruft hanging around, but for backwards compatibility I suppose a new variable is the only way.

To clear up any potential confusion -

Any newer Mudlet can load any older map version (and then it saves it in the current map format)
No older Mudlet can load a newer map version

So the practical effect is that we can change the map version format, just not very often, because upgrading Mudlet isn't that seamless (as say the self-updating Chrome). As soon as the person doing the map upgrades (after holding out appropriately long and missing out on the new features) upgrades, everyone else on the older versions has to.

I'm going to merge this in then because I like what's been done for the exit mapping and the actual code is cleaner. But I'm going to make new versions of mudlet empty the exits table since it's useless overhead (but still there/saved for compatibility) with a warning in the code of why there's a dead variable hanging around. We can revisit it at a later Mudlet. A good lesson in how wise it is to put data structures that are wrong/may change in the map file.

Didn't @SlySven mention this has an issue of only allowing 1 special exit between two rooms?

It does. If @SlySven doesn't mind, I'll pull this locally and submit a new PR with the faulty exits being removed and a new exit mapping being created.

@Chris7 This latest commit should sort things out - it provides an additional TArea Class method to massage the area exit data to the wanted QMap<(int)fromRomId, QPair<(QString)direction, (int)toRoomId> > and uses it when TLuaInterpreter::getAreaExits() is called with a second optional boolean argument set to true. I think this will render #225 redundant. 😄

I anticipate that this will/can be revisited when/if we restructure the TRoom::other member to use the exit direction as the key instead of the toRoomId.

It also renames those areaExits finding methods to something in English!

How do you think I should best present the directions in the detailed output for that lua function? 😕

Same way as http://wiki.mudlet.org/w/Manual:Mapper_Functions#getRoomExits does, without the translation stuff as it has.

Can I make the defensive point that just because a string is marked as being a candidate for translation that it does not have to be translated, we are perfectly at liberty to leave the "translated" version to be the same as the source when Qt Linguist is used - or indeed the person doing it could produce two translations to ship: one with the lua API using the US English form and one completely rendered in their mother tongue - it may be that we recommend the former but a section of users might wish to go all the way (possibly at their own effort) with the latter. This, I would imagine, could be a selling point for some countries (somehow I keep thinking of France for some reason, of course they'd be wanting the cardinal compass points to be n,e,s,o !)

For the moment I see that we'd get what you describe if I took the hyphens out of the diagonals...

I don't think APIs in other languages are an idea that took off. Microsoft has translated APIs with Excel formula names I know, and as far as I can see that just makes coding harder for everyone involved.

The other issue is adding this stuff doesn't guarantee that's what we'll end up with once we decide to take it seriously. So why have half of the stuff translatable in various different forms only for us to decide on something and have to go through and fix everything? Makes more sense to put it off until we're ready to tackle it, form a strategy and apply it.

Humm, this has become a series of four commits but it would make things clearer if I squashed the last three together IMO. As I understand it that would make the git history cleaner when it gets merged but can be confusing if anyone looks through this PR. @vadi2 Are you OK with the state now before I squash the last two commits into the second one?

Yep, go ahead!

OK - should be just the two commits ready for someone to merge now. 👍 {Also kills off need for #225 I believe!}

Been tweaking again, to get the lua returns inline with the suggestions given for error/fault/no output cases. Bottom line:

• User supplies wrong type of argument: lua_error is thrown, returning a string diagnostic message.
• User supplies right type of arguments, but values are faulty: e.g. negative RoomID: nil (primary) return value plus (secondary) string diagnostic message.
• User supplies right values but no output: empty table or string or signal number returned.
• User supplies right values but function has no intrinsic output: return boolean true to indicate success.

If there are no objections I'd like to get this one into development branch.

Note: I've raise the point on Gitter that we are ambiguous about duplication of area names - some parts demand unique area names others permit them without hesitation. I have another Pull Request ready to put out that addresses this consistently (prohibits duplicates, fixes existing maps by applying numeric suffixes to duplicates, and applying a generic name to unnamed areas {with a suffix if more than one} - it advises user with what is happening and why and offers them the information of the specific area names before and after that are being affected) and it will only hit the user once per map file. I've thought about this a little and I feel that allowing two areas to have an identical name does not make sense from a semantic point of view and whilst it might surprise a user who falls foul of this issue if they are referring to areas by name as some of the Lua functions permit they are gonna have bigger problems than existing names changing in a comprehensible manner.

Unfortunately the fix I have for this does merge conflict slightly with this one so as to save having two versions here I won't raise a PR until this one has had a final 👍 or 👎 .

The two candidates are is: against current development branch or post application of this one post application of this one.

Looks good to me otherwise.

This needs to be merged into release_30 because it fixes getAreaExits(), a function introduced in 3.0 but was broken.

OK, so I should make a local branch of release30 and check that I can merge the same series of commits in on my system then put up a PR against that branch, yes?

Yeah

Sent from my phone
On Jan 6, 2015 10:59 AM, "Stephen Lyons" notifications@github.com wrote:

OK, so I should make a local branch of release30 and check that I can
merge the same series of commits in on my system then put up a PR against
that branch, yes?

—
Reply to this email directly or view it on GitHub
#219 (comment).

@Chris7 Done, awaiting merge by a suitably authorised person 😉