Skip to content

Ping categories, persistent markers, selection UI, and per-player/faction visibility filters#927

Open
LumiLunaLuma wants to merge 2 commits into
rwmt:devfrom
LumiLunaLuma:ping-marker-expansion
Open

Ping categories, persistent markers, selection UI, and per-player/faction visibility filters#927
LumiLunaLuma wants to merge 2 commits into
rwmt:devfrom
LumiLunaLuma:ping-marker-expansion

Conversation

@LumiLunaLuma
Copy link
Copy Markdown

@LumiLunaLuma LumiLunaLuma commented May 19, 2026

Summary

Adds typed pings and persistent on-map markers. Players hold to open a radial wheel, pick a category, and release to send a ping or place a marker. Markers can be renamed or deleted by their owner or by the host, and each client can adjust marker opacity locally. A per-client filter hides markers by faction or by player. The host has a per-player marker cap and clear-all controls.

What changed

  • Six ping categories: Default, Attack, Defend, Help, Loot, Rally
  • Radial selection wheel on hold; release sends a ping or places a marker depending on the per-client place mode
  • Draggable menu window with the wheel on the left and a markers list on the right; click a row to jump to that marker
  • On-map click selects a ping or marker and opens an inspect pane
  • Marker rename and delete (owner or host), with per-client opacity
  • Per-client visibility filters by faction and by player, plus a master toggle for spectator markers
  • Host settings dialog with a per-player marker cap (synced via MultiplayerGameComp) and clear-all controls
  • New keybind MpTogglePingMenu with no default key, to avoid colliding with mod-added bindings

Wire and protocol

  • MpVersion.Protocol bumped from 55 to 56
  • PingLocationPacket carries a PingCategory byte and a UTF-8 label of up to 64 chars
  • New paired client/server packets: ClearMarkers, DeleteMarker, RenameMarker

Persistence

  • Markers are scribed on MultiplayerGameComp, so they're included in every save and in replay sections
  • ConvertToSp drops MP-only state (including markers); the pre-conversion replay preserves it for re-hosting
  • New MpSettings fields cover the wheel toggle, hold delay, place mode, last-used category memory, three window rects, the spectator-marker toggle, hidden factions and players, and per-marker opacity and visibility overrides. New scribe entries use an mp* XML prefix.

…tion visibility filters

Adds typed pings and persistent on-map markers.

Players hold to open a radial wheel, pick a category, and release
to send a ping or place a marker. Six categories: Default, Attack,
Defend, Help, Loot, Rally.

Markers can be renamed or deleted by their owner or by the host.
Each client can adjust marker opacity locally.

A per-client filter hides markers by faction or by player, with a
master toggle for spectator markers. The host has a settings
dialog with a per-player marker cap and clear-all controls.

Wire protocol bumps from 55 to 63. PingLocationPacket now carries
a PingCategory byte and a UTF-8 label of up to 64 chars. New
paired client/server packets: ClearMarkers, DeleteMarker,
RenameMarker.

Markers are scribed on MultiplayerGameComp, so they're included
in every save and in replay sections. ConvertToSp drops MP-only
state (including markers) on the way out; the pre-conversion
replay preserves it for re-hosting.

Per-client preferences (wheel toggle, hold delay, place mode,
last-used category memory, window rects, hidden factions and
players, spectator-marker toggle, and per-marker opacity and
visibility overrides) live in MpSettings under an mp* XML prefix.

New keybind MpTogglePingMenu with no default key, to avoid
colliding with mod-added bindings.
@notfood notfood added enhancement New feature or request. 1.6 Fixes or bugs relating to 1.6 (Not Odyssey). labels May 19, 2026
@notfood notfood moved this to In review in 1.6 and Odyssey May 19, 2026
Copy link
Copy Markdown
Member

@notfood notfood left a comment

Choose a reason for hiding this comment

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

From a cursory glance, it's looking good.

Instead of hardcoding the categories, it'd be nice to have it available in XML to allow more freedom for other mods to add stuff. Shouldn't be too complicated to make it dynamic. Just have PingCategoryExtensions read some <MultiplayerPingDef>.

Is the protocol leap necessary? Could be just +1.

@LumiLunaLuma
Copy link
Copy Markdown
Author

From a cursory glance, it's looking good.

Instead of hardcoding the categories, it'd be nice to have it available in XML to allow more freedom for other mods to add stuff. Shouldn't be too complicated to make it dynamic. Just have PingCategoryExtensions read some <MultiplayerPingDef>.

Is the protocol leap necessary? Could be just +1.

Apologies with the protocol leap, you are correct it needs to just be +1, during dev I bumped each time I made a wire-incompatible change so that I could make sure I never connected with a test client on an older build, I'll bring it down to 56.

Will start working on a follow-up commit with the fix for the protocol leap and also implement the dynamic categories, should be a fairly simple update for it, just going to need to play around with the wheel but have a neat idea that I'm going to try out for the UI 😁

@LumiLunaLuma LumiLunaLuma force-pushed the ping-marker-expansion branch from 7371f30 to 7f3c427 Compare May 20, 2026 07:06
Comment thread Source/Client/UI/AlertPing.cs Outdated
using Verse;

namespace Multiplayer.Client
namespace Multiplayer.Client;
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.

Please revert to block-scoped namespace for consistency across the project. File-scoped namespaces should be introduced in a separate, project-wide PR if we decide to go that route.

var diffAt = FindTraceHashesDiffTick(local, remote, out var found);
Multiplayer.Client.Send(new ClientDesyncedPacket(local.startTick, diffAt));

var snapshotInfo = TryRefreshSnapshotForDesync();
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.

Any more details about this feature?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure no problem. I primarily used this on my side to get quicker debug info on desyncs I was hitting during dev. Essentially it gives you a quick snap point to when the desync got detected which is usually a few ticks after the actual divergence, but there's a Snapshot Lag Ticks field in the desync_info that tells you relatively how far behind it ended up. So you can load up the replay and look at what the world state was around the bad tick, instead of from the last cached snapshot.

Regarding what info it actually gives you, you can extract the replay.rwmts from the desync zip and use dev tools to inspect what was happening at that moment. For the marker work that was mostly visually checking which markers each client actually had on the map at a bad tick, combined with the stack traces in local_traces.txt that gave me a quick way to spot something like "client 1 has marker X but client 2 doesn't" without having to manually step to get there.

Implementation wise it's basically wrapping SaveLoad.SaveGameData + CreateGameDataSnapshot to grab the current state locally, without sending anything to the server or triggering a join point for other players. Runs on the main thread since I believe Scribe and Find.Maps aren't thread-safe.

Probably should've taken this out of the PR though, it was actually just a quick thing I added to get more debug info on my side and isn't really related to the category-pings PR. The other thing is, the way it's done currently means the embedded replay in the desync zip starts at the desync tick instead of the earlier cached snapshot, so you lose the ability to forward-sim from that earlier point and see the points leading up to the bad ticks, which is most probably more useful than the snapshot of information I was using. I was actually thinking of doing this as a separate PR where we maybe expand the desync zip to include both the snap-to-the-point snapshot for quick debugging of the divergence itself and then the full replay version for seeing what led up to it. Would you like me to remove this from the current PR and possibly create a new PR for it or just scrap it fully?

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.

It's a nice idea, but let's break this out into a separate PR, it's out of scope here and could use some refinement.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Noted, will be pushing a commit soon with updates for all of your comments 😁

Comment thread Source/Client/UI/LocationPings.Wheel.cs Outdated
private const float ChevronTabGapY = 4f;

// Clockwise from the top.
private static readonly PingCategory[] WheelOptions =
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.

Duplicate from PingCategoryExtensions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Resolved duplications in XML category restructure commit.


namespace Multiplayer.Client;

public static class PingCategoryExtensions
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.

Would be nice to load by XML to allow modding and extension.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Categories now defined as XML, added pagination to the radial picker when more than 6 categories present.

Screenshot 2026-05-20 195117 Screenshot 2026-05-20 195141

Comment thread Source/Client/Util/MpTranslate.cs Outdated
namespace Multiplayer.Client.Util
{
// English fallback for keys not yet shipped in rwmt/Multiplayer-Locale.
public static class MpTranslate
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.

Ideally, we don't need this. Allow it to fail, it should be shipped with the right Multiplayer-Locale.

// English string and dev-mode pseudo-localization mangles the result. Inject the value into the
// active language so the normal Translate() path resolves cleanly until the keys land in the
// Languages submodule.
public static class PingRuntimeTranslations
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.

Assume it runs with the right Multiplayer-Locale, the only scenario where it fails is between commits until it's merged and that shouldn't reach the user. This isn't needed.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Removed fallbacks, will get a PR ready for Multiplayer-Locale with translations.

using Multiplayer.Common.Networking.Packet;

namespace Multiplayer.Common
namespace Multiplayer.Common;
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.

Revert to block-scoped namespace

Comment thread Source/Common/Version.cs Outdated
public const int Protocol = 55;

// Wire-compatibility protocol version; intentionally distinct from Packets.Max.
public const int Protocol = 63;
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.

Protocol 56

Moved ping categories into XML and added pagination to the radial wheel.

Categories now live in Defs/MultiplayerPingDefs.xml as
MultiplayerPingDef defs with label, description, order, tint,
icon, and sound. The six vanilla categories reacreated in XML format.

The wheel pages when more than six categories are present.
Back and More chevron slices sit at the upper-left and
upper-right of the wheel, with dots above the wheel for page indication.

Wire protocol bumps from 55 to 56. PingLocationPacket now
carries a ushort def short-hash instead of a byte enum. Markers scribe by
defName, so save/load survives categories being added or removed.

Namespaces in every new file this PR introduced are reverted
to block-scoped.

Drops the in-code English-fallback helper and the runtime
keyed-replacement injection.

The desync-snapshot diagnostic in SyncCoordinator removed.
Copy link
Copy Markdown
Member

@mibac138 mibac138 left a comment

Choose a reason for hiding this comment

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

This is a big PR and these are the things that stood out the most to me. I did not yet review it fully. As a side note, it'd be nice to have some images or a simple demo video with the new features showcased :)

Comment on lines +115 to +118
var comp = Multiplayer.game?.gameComp;
var markerCount = comp?.AllMarkers.Count.ToStringSafe() ?? "n/a";
var nextMarkerId = comp?.nextMarkerId.ToStringSafe() ?? "n/a";
var markerCap = comp?.markerCapPerPlayer.ToStringSafe() ?? "n/a";
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.

Do we even want to add this info to the desync report? Not sure how this is useful

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I originally added these for debugging marker specific desyncs during dev. The idea was that if you have two players with different Marker Count or Next Marker Id in the desync report, you immediately know the divergence happened somewhere in the marker path. The "markerCap" was mainly just there for me to make sure that the marker cap set by the host is properly syncing to all clients. The info doesn't really provide any other info regarding desyncs so I fully understand if you'd prefer it removed.

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.

What's the point of the midjoin packet code? You are sending the packets as reliable, so if you send it to joining players too, then once the loading has finished, they should just normally receive the packet. This approach works fine for SyncMethods which are critical to keep the game in-sync, so it should work fine here too

Comment on lines +26 to +29
public int lastPingTick = -1;
public int lastMarkerClearTick = -1;
public int lastMarkerDeleteTick = -1;
public int lastMarkerRenameTick = -1;
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.

Why?

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.

Why is the markerCap added here? I couldn't find any uses of it

Comment on lines +7 to +9
public int playerId = playerId;
public int factionId = factionId;
public string username = username;
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.

Why are you sending both the playerId and username? Player id should be enough. This also applies to the other new packets

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

1.6 Fixes or bugs relating to 1.6 (Not Odyssey). enhancement New feature or request.

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants