You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Every claim below was reproduced against real Mudlet 4.20.1 (headless, xvfb, the actual package loaded as a profile) with the real mapper API and real HTTP stack. Nits and unverifiable claims are omitted.
1. Failed mapper calls are published to the service as if they succeeded
mudlet-mapper.xml:6196-6386 - every wrapper guards with if result ~= false then service.submit(...).
Mudlet signals mapper failure with nil, message, not false (TLuaInterpreter::warnArgumentValue, TLuaInterpreter.cpp:320-348, does lua_pushnil; lua_pushstring; return 2). nil ~= false, so the guard never fires.
Measured which of the 20 wrapped functions actually return false on failure:
17 of 20 are unguarded in practice. Reproduced end-to-end:
unwrapped setDoor(701,'east',2) -> nil , roomID 701 does not have a special exit in direction 'east'
getDoors(701) after = [] <- local map genuinely unchanged
wrapped setDoor(701,'east',2) -> POST {"type":"set-exit-door","roomNumber":701,"direction":"east","status":2}
unwrapped setRoomArea(701, 4242) -> nil , number 4242 is not a valid area id
getRoomArea(701) = -1 <- unchanged
wrapped setRoomArea(701, 4242) -> POST {"type":"set-room-area","roomNumber":701,"areaId":4242}
three wrapped calls on nonexistent room 888888 -> 3 POSTs
{"type":"set-room-environment","roomNumber":888888,"environmentId":5}
{"type":"set-room-symbol","roomNumber":888888,"symbol":"X"}
{"type":"set-room-hash","roomNumber":888888,"hash":"abc"}
Note setDoor(room, "east", ...) is a realistic call shape that Mudlet rejects (it wants "e" for normal exits) - and the directions table at 6165 passes "east" through, so this specific combination always reports a door that was never set.
This also contradicts README.md:38-39: "A successful local mapping operation does not depend on the service: your local map is changed first."
Fix: if result then, and handle clearSpecialExits / setRoomIDbyHash / deleteMapLabel explicitly (they return zero values, so they always report). addAreaName at 6198 already does this correctly with if areaId then.
2. One redirect permanently wedges the send queue - silently
mudlet-mapper.xml:6136-6140 (service.sendNext) and 6402-6415 (the two handlers compare url == endpoint("/change")).
Mudlet applies NoLessSafeRedirectPolicy to every postHTTP and passes reply->url().toString() - the final URL - to the completion event. A 301 on a POST also turns it into a GET, so sysGetHttpDone fires and neither handler is listening at all.
Reproduced against a local server that 301s :8898/change to :8899/change:
crowdmapserviceurl = http://127.0.0.1:8898
inFlight immediately after submit = true, queued = 0
events Mudlet actually raised:
sysGetHttpDone url/arg1=http://127.0.0.1:8899/change
handler expected url == "http://127.0.0.1:8898/change"
service.inFlight AFTER the request completed = true <- never cleared
3 more changes -> pendingChanges = 3, inFlight = true
after 3 more seconds -> pendingChanges = 3, inFlight = true
control (no redirect) -> pendingChanges = 0, inFlight = false <- drains normally
Second wedge path, same root cause - changing the URL while a POST is in flight:
No error is shown in either case. http:// URLs pass the option validator (value:match("^https?://"), line 5955), so mconfig crowdmapserviceurl http://achaea.mudmaps.community is enough to trigger this against any host that upgrades to https.
Secondary: service.inFlight = true is set beforetable.remove and endpoint() are evaluated, so a throw there loses the popped change and wedges the queue.
Fix: postHTTP returns true, actualUrl - capture and compare that, listen for sysGetHttpDone too, and add a watchdog timer.
3. Private room marks are uploaded to the community map
mudlet-mapper.xml:5644-5675 (room mark) and 5682-5690 (room unmark) write setRoomUserData(1, "gotoMapping", ...) without mmp.localMapChanges. Both addRoom and setRoomUserData are globally wrapped.
Captured payload with mapsource=service, crowdmapservicesend=on:
Everyone's private mark list goes out under the same key on room 1, so participants overwrite each other, and downloading the service map replaces your marks with whoever reported last. On maps without room 1, a phantom room 1 is created and published too.
This is inconsistent within the PR itself: commit 392aefa deliberately wrapped the migration writes of the same key (12783-12785, 12818-12820) in mmp.localMapChanges, but left the primary user-facing write paths unwrapped.
4. endpoint() throws out of the global HTTP handlers when the game isn't known yet
mudlet-mapper.xml:6114-6120 with 6402-6415. mmp.game = false until a login trigger fires (line 422 for Achaea). Both handlers call endpoint("/change") unconditionally on everypostHTTP completion anywhere in the profile, with no mapsource guard and no pcall.
Reproduced with default settings, any other package doing a POST:
[ LUA ] - error in event handler for sysPostHttpDone:
<[string "Script: Crowdmap service"]:8: Cannot derive the crowdmap service URL
before the game is known.>
On a profile where the mapper never identifies the game, this fires for every HTTP POST for the whole session.
5. Lusternia transverse/pathfind exits are broadcast to the shared map
mmp.registerPathfind (12028), mmp.clearPathfind (12036), mmp.registerTransverseExit (11997), mmp.clearTransverse (12009), mmp.lockpaths (12055) - none wrapped in mmp.localMapChanges. These are per-character, seconds-long skill exits driven by triggers at 3110/3133/3155/3179.
mmp.lockpaths is the one sibling of those five that was missed.
6. Room-update feedback messages and the map recentre were dropped
mmp.syncSafeRoomInfo (6421) replaced inline blocks in mmp.mappingNewroom (call site 11099). The surrounding if #s > 0 then mmp.echo(s); centerview(mmp.currentroom) end (11101-11104) is unchanged, but the extracted code no longer appends to s - and s is now only fed by exit-related messages.
These strings exist on master inside mmp.mappingNewroom and exist nowhere in the branch:
master:10580 "Updated room name to '" .. rootroomname .. "'."
master:10728 "Updated environment name to " ...
master:10738 "Updated room to be indoors."
master:10742 "Updated room to be outdoors."
master:10748 "Updated game area to " .. serverArea .. "."
master:10757 "Added the wilderness mark."
Empirically, the new function does the work but says nothing:
console lines emitted by syncSafeRoomInfo: 0
but it did change: name=Brand New Name char="W" indoors="y"
So walking into a room whose name changed in game silently renames it, with no message and no centerview. (unHighlightRoom was correctly preserved; only the echoes and the recentre were lost.)
7. createMapLabel publishes temporary labels as permanent
mudlet-mapper.xml:6354-6379. Mudlet takes temporary as argument 19 (TLuaInterpreterMapper.cpp, args > 18 -> getVerifiedBool(L, __func__, 19, "temporary")) and getMapLabel returns it as label.Temporary. The wrapper reads neither - it only picks arguments 1, 14, 15, 17, 18.
unwrapped createMapLabel(..., temporary=true) -> id 0, getMapLabel().Temporary = true
wrapped createMapLabel(..., temporary=true) -> id 1, getMapLabel().Temporary = true
payload: {"type":"set-map-label","areaId":1,"labelId":1,"label":{...}} <- no temporary flag
labels in area before saveMap/loadMap: 2
labels in area after reload: 0 <- Mudlet drops temporary labels
A label the reporter's own map will not even keep is published to everyone as permanent. The wrapper could read label.Temporary / label.OnTop / label.Scaling from getMapLabel instead of guessing from varargs.
8. "No wormhole" trigger deletes an unverified reverse exit and reports the failed deletion
Nothing checks with getSpecialExits(wormholeDestination) that the destination's wormhole leads back here, and the body is not inside mmp.localMapChanges. Reproduced:
before: 901 special exits = {"worm warp":"0"}
before: 902 special exits = [] (no wormhole back)
unwrapped removeSpecialExit(901,'worm warp') -> true, nil
unwrapped removeSpecialExit(902,'worm warp') -> nil, the special exit name/command
'worm warp' does not exist in exit roomID 902 <- no-op locally
the two wrapped calls the trigger makes -> 2 POSTs:
{"type":"delete-special-exit","roomNumber":901,"exitCommand":"worm warp"}
{"type":"delete-special-exit","roomNumber":902,"exitCommand":"worm warp"} <- finding 1 in action
If the destination's wormhole points somewhere else, a still-valid exit is destroyed locally and the deletion is published; if it has none, a deletion that did nothing is published anyway.
(The behaviour change from lockSpecialExit(..., true) to outright removal appears intentional per commits 9a7b0e9 / e6bf884 - flagging only the unverified reverse exit and the failed-call reporting.)
9. mmp.achaeaBetterWings is silently dead when mapsource == "service"
mudlet-mapper.xml:7981-7991. The old gate was (not mmp.settings.crowdmap) or; the PR translated it as (mmp.settings.mapsource ~= "published") or. Measured:
The very next line calls orbedBlackList() -> mmp.getAchaeaOrbTable() (11349), which the same PR translated as mmp.settings.mapsource ~= "game" - i.e. it treats service as a crowdmap. I confirmed getAchaeaOrbTable picks the crowdmap table under service (#orb.ashtan == 2), so the two gates genuinely disagree. Fix: (mmp.settings.mapsource == "game") or.
Not reproduced / excluded
I dropped the reporter-identity race (upload uses gmcp.Char.Status.name, download may use getProfileName()) - it depends on real IRE GMCP arrival timing I could not exercise. Load order / wrapSetExit, the option system and its validators, mmp.migrateOptions, the JSON-quoted version parsing, queryValue percent-encoding and the directions table all checked out correct.
This PR integrates the mapper with Crowdmap for configurable map sourcing and change reporting while adding local-state handling and GMCP synchronization.
Changes:
Adds configurable live, published, and service map sources.
Adds queued service reporting and local/private state handling.
Documents service settings, room marks, and GMCP updates.
File
Summary
Review status
README.md
Documents map sources, service settings, room marks, and GMCP updates.
No final findings.
mudlet-mapper.xml
Implements Crowdmap integration, reporting, synchronization, and local-state handling.
1 critical, 13 moderate, and 1 nit finding remain unresolved.
The queued report is removed before the HTTP response arrives. If the request later raises sysPostHttpError or times out, the completion path only logs and advances, so the successful local edit's report is permanently discarded; keep it queued until success or reinsert it on failure/timeout.
Wildnode toggles publish local routing links
mudlet-mapper.xml:10666
Installing this wrapper makes the nodes on/off alias's mmp.wildnodes() calls submit modify-exit/delete-exit reports, because that function calls mmp.setExit directly. These optional per-character routing links are not inside mmp.localMapChanges (unlike astroboots), so enabling service sending publishes them as shared map topology; suppress reporting while applying this toggle.
Legacy mark migration overwrites existing private marks
mudlet-mapper.xml:12890
An existing private mark with this name is loaded into oldPrivateMarks before this loop, but this assignment overwrites it whenever the legacy public mark differs from the new map. An update can therefore silently replace a user's private mark; only migrate the legacy value when no private value already exists.
This issue also appears on line 12929 of the same file.
Reverse wormhole cleanup checks only an arbitrary exit
mudlet-mapper.xml:375
next(reverseExits[mmp.currentroom]) examines only an arbitrary first command. If that destination has another special exit before worm warp, the reverse wormhole is left in the map even though this path is being removed, so future pathfinding can still select the broken exit. Test for the "worm warp" key directly before removing it.
Empty optional captures mishandle room mark visibility and location
mudlet-mapper.xml:5650
The optional visibility and location captures are not normalized for Mudlet's unmatched-capture value. In this file optional alias captures are handled as empty strings (for example, the rw/rwe aliases), so room mark home can pass an empty visibility to mmp.getRoomMarks/setRoomMark and take the empty-string location branch instead of using the current room. Treat both nil and empty captures as absent before branching.
This issue also appears on line 5664 of the same file.
Unknown hashes publish phantom rooms to the service
mudlet-mapper.xml:6239
mmp.roomidFromHash calls addRoom and setRoomIDbyHash to create a hash-only placeholder, then marks it with hashonly; mmp.roomexists explicitly treats that placeholder as nonexistent. These wrappers still submit create-room and set-room-hash, so merely seeing an unknown hash during normal tracking can publish a phantom room to the service. Keep the placeholder creation/hash binding local until the room is promoted to a real mapped room.
Missing room details incorrectly marks rooms as outdoors
mudlet-mapper.xml:6554
The or {} fallback makes missing Room.Info.details indistinguishable from an outdoor room. When that field is absent, indoors is false and enabling gmcpmapupdates clears any indoor mark and writes outdoors=y for every synchronized room. Only run this indoor/outdoor update when info.details is actually a details table.
Switching back to service leaves queued reports stuck
mudlet-mapper.xml:9920
Queued reports are intentionally retained when mapsource is changed away from service, but this branch never resumes the queue when the user switches back. With an already-known reporter and sending still enabled, no status event or new edit may occur, so the retained reports can remain stuck indefinitely; call sendNext() when selecting the service source.
Version marker advances before map download succeeds
mudlet-mapper.xml:13244
The service path records the new version before downloadFile starts. If this map request fails, the next version check sees the version as current and skips the download, leaving the old map indefinitely (unless the user notices and manually retries); advance the marker only after the map has downloaded and loaded successfully, or clear it on failure.
I reviewed this PR and checked each finding below by running it. The test setup was a real Mudlet under xvfb, the PR's mudlet-mapper.xml installed into a profile, a local mock Crowdmap service, and busted specs. Every item here reproduced. Line numbers refer to mudlet-mapper.xml at 4beadf2.
Bugs
1. Updating the mapper in-session loads the published crowdmap over a game-map user's map mmp.installMapperScript reinstalls the package without a restart. mmp.firstRun is already false, so mmp.startup() returns early (5923) and the new mapsource option is never created. mmp.settings.mapsource is then nil, and nil counts as "not game" in needupdate (13053), getAchaeaOrbTable (11528) and achaeaBetterWings (8147).
Reproduced: installed master, then uninstalled and installed this PR the way the self-update does. mapsource was nil. With updatemap on and a changed MD5SUM, the next mmp.checkforupdate() fetched AchaeaCrowdmap/Map/changelog.txt and AchaeaCrowdmap/Map/map, then printed "Map downloaded, loading it in…".
A fresh install of the PR correctly took the "The games map was updated" branch.
getAchaeaOrbTable().ashtan also changed from {49,53,54,60} to the crowdmap {49,53}.
Suggestion: read the source through one helper that falls back to "game" (or to "published" if the old crowdmap flag is set), or register missing options when a newer script loads over an older one.
2. The global map-function wrappers change Mudlet's API for every script in the profile (6246–6447)
The wrappers are installed whatever mapsource is, and each one takes a fixed argument list and returns a single value.
addRoom(id, areaId) drops areaId. Reproduced: the room landed in area -1; the original function put it in area 1.
Failure reasons are lost. Reproduced:removeSpecialExit(987654321, "nope") returns {} through the wrapper, but {nil, "number 987654321 is not a valid exit roomID"} from the original.
Suggestion: take ... and return every result, e.g. local r = {original.fn(...)} … return unpack(r, 1, table.maxn(r)).
3. Deleting the last label in an area is never reported (6439–6446)
Once an area has no labels, getMapLabel(areaId, labelId) returns {}, not nil. So not getMapLabel(...) is false and the delete is skipped. This also affects mmp.clearLabels.
Reproduced: created labels 0 and 1, then deleted both. Only delete-map-label:0 was submitted. The service keeps label 1, and the next download brings it back.
Suggestion: treat next(getMapLabel(areaId, labelId) or {}) == nil as deleted.
4. A short outage loses every report made during it (6100, 6463)
Retries use a fixed 1-second delay and give up after 3 attempts.
Reproduced: with the connection refused, one setRoomName went through three attempts and "Discarded after 3 attempts." in about 2 seconds.
The queue is also memory-only, so anything pending is lost when Mudlet closes.
Suggestion: add backoff, and keep the change queued instead of discarding it.
5. Unsent local edits are overwritten by the next service map download
The README says "If submitting the report fails, the mapper displays an error and the local edit remains". That only holds until the next map download.
Reproduced: set a room name locally; the service returned 500 three times and the change was discarded. The next mmp.checkforupdate() loaded the service map, and the room name went from my-local-edit back to server-name with no warning.
Suggestion: at least warn when unsent or discarded changes exist before loadMap, and correct the README.
6. Public room marks are reported as one JSON value, including older marks (6519–6546, 6505) room mark public bank 5678 sent value = {"legacyhome":42,"bank":5678}. Marks the user made before this PR are still stored under the now-public gotoMapping key, so they are uploaded with the first public mark. Each report also replaces all public marks as one value, so two players' marks can't combine.
Malformed gotoMapping data (for example from the service) makes mmp.getRoomMarks throw InvalidJSONInput, because yajl.to_value has no pcall. That breaks room marks and goto <mark>.
Suggestion: send one report per mark, and decode through pcall.
Documentation and help text
7. README lines 6 and 24: HTML entities inside code spans render literally. GitHub shows <source> and <game>. Pasting the URL from line 24 into mconfig crowdmapserviceurl produced requests to the literal host https://<game>.mudmaps.community/…. Use <source> and <game> unescaped.
8. The <game> placeholder only works for the exact default URL (5938 vs 6102–6108). The help text says "Use to derive the subdomain", but endpoint() only substitutes it when the whole URL matches the default string.
Reproduced:https://<game>.example.org stayed literal. So did the default URL with a trailing /.
Suggestion: use url:gsub("<game>", mmp.game), or reword the help text.
9. README lines 55–57 say your crowdmapservicereports threshold decides what other mappers see. The threshold is never sent with a report.
Reproduced: with the threshold set to 5, the report POST had no threshold, and only the map download URL carried timesSeen=5.
Each mapper's own threshold controls only their own download, which lines 50–51 already describe correctly.
10. README lines 77–79 say only "legacy" marks are migrated to private. In fact every public mark missing from the downloaded map is demoted to private, including ones just made.
Reproduced: with mapsource published, ran room mark public freshpublic 1234, then completed a published-map download. The mark was listed as private with no message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification