fix(mp): harden Select Similar exemption + add opt-in diagnostic (dev build for repro) - #29
Merged
Merged
Conversation
…tiCell The unpatch approach removed Multiplayer's prefixes from the three Designate* overrides declared on Designator_SelectSimilar, but DesignateMultiCell calls base.DesignateMultiCell, and Multiplayer also patches Verse.Designator's own Designate* methods (every declared override on every subtype, base class included). A base call goes straight through the detour on the base method, so drags were still serialized and cancelled locally even with 3/3 overrides unpatched, then replayed from the tick loop without the cursor/selection context the filter needs — failing with "No Selectables". Prefix Multiplayer.Client.DesignatorPatches.Designate* themselves instead: they receive the designator instance for every patched route, base calls included, so a single instance check covers all paths. Runs before the MP.enabled gate since it needs no Multiplayer API. Same pattern Multiplayer-Compatibility uses for Allow Tool's select similar.
PR #28's SkipSyncForSelectSimilar was reported by a tester as still failing to prevent Select Similar drags from being command-synced, despite the "sync bypassed on 3/3 designate hooks" install log firing successfully. The install succeeds but the drag is not exempted at runtime. Two changes to narrow the failure mode without a third speculative fix: 1. Swap [HarmonyArgument("__instance")] Designator designator for the positional Designator __0. Multiplayer's DesignatorPatches.Designate* are static methods whose first parameter is literally named __instance, which collides with Harmony's __instance special name for the instance-of-a-nonstatic-target (null for a static target). __0 is Harmony's unambiguous positional accessor for the target's first declared parameter and sidesteps the collision on every Harmony 2.x version KAU or its cohabiting mods might ship. Removes hypothesis 1 (arg resolves to null) as a variable. 2. Add Settings.MpDebugLogging (default off, exposed under a new Diagnostics section in the mod settings UI). When on: - SkipSyncForSelectSimilar logs one line per interception with the patched method name, designator instance type, and chosen branch. - Designator_SelectSimilar.DesignateMultiCell logs at entry so we know whether a drag under MP actually reaches the KAU override or is intercepted at a higher layer. The two logs together disambiguate between hypotheses 2 (MP intercepts drag before DesignatorPatches — e.g. an input-layer or DragBox sync path) and 3 (our detour on the prefix method fails to intercept the wrapper's call). Reporter can flip the toggle, do one drag, share the log.
The compat folder was already loading in production (Discord reporter's log shows "sync bypassed on 3/3 designate hooks" firing, which requires the DLL to have loaded). No bug fix needed here — just tightening. Change: match value flipped to lowercase "rwmt.multiplayer" to mirror RimWorld's runtime-normalized packageId (ModLister.RunningModsListForReading shows the lowercase form even when About.xml declares "rwmt.Multiplayer"). IfModActive is case-insensitive against the Active mod set in production, so both casings work — this is defensive, not corrective. Comment records the evidence so a future maintainer doesn't retrace the same investigation. Note: RimMCP's game_start mod-list override loads assemblies but leaves ModMetaData.Active=False, which breaks IfModActive during local test — that's a RimMCP env quirk, not a KAU packaging bug.
Owner
Author
Local repro proved the exemption worksTraced through with RimMCP (five-mod list: base + Harmony + Multiplayer + KAU + RimMCP), joined an in-process MP session, force-loaded the compat DLL via REPL (the compat folder didn't auto-load under RimMCP's mod-list override — a RimMCP env quirk where
Result: 25 chunks selected as expected. Log lines: The second-order patch on Why does the reporter still see a failure?They saw PR #28's
Ask reporter to re-test this build
Decision matrix in the PR body maps their output → next fix. If they get |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Draft. Rebased on top of #28's approach — not intended to merge until we have a reporter log confirming which failure mode we're on.
Background
3/3 designate methods unpatchedin log but drags still failed.Multiplayer.Client.DesignatorPatches.Designate*(mirrors Multiplayer-Compatibility's AllowTool exemption). Reporter tested that too and saw the newsync bypassed on 3/3 designate hooksinstall-success line — but drag still failed.What this changes
1. Harden
SkipSyncForSelectSimilararg resolution[HarmonyArgument("__instance")] Designator designator→Designator __0.Multiplayer's
DesignatorPatches.Designate*are static methods whose first parameter is literally named__instance. That collides with Harmony's__instancespecial-name (target-instance, which is null for a static target). Different Harmony 2.x versions have handled that collision differently.__0is Harmony's unambiguous positional accessor for the target's first declared parameter and dodges the whole class of failure mode.Same semantics, removes hypothesis "arg comes through as null" as a variable.
2. Opt-in diagnostic logging
New
Settings.MpDebugLogging(default off), exposed under a new Diagnostics section in the mod settings UI. When on:SkipSyncForSelectSimilarlogs one line per invocation with method name, designator instance type, and chosen branch (bypass-syncvslet-mp-sync).Designator_SelectSimilar.DesignateMultiCelllogs at entry so we know whether the drag actually reaches the KAU override at all under MP.Decision matrix from one test drag
decision=let-mp-syncdecision=bypass-syncdecision=bypass-syncWhat this does NOT change
DesignatorPatches) — same as AllowTool's reference pattern.Settings.DesignateSuccessFeedbackSuppressed+ explicitsoundSucceeded.PlayOneShotOnCamera()in the drag override. NoDesignator.Finalizepatch needed here, unlike AllowTool.Test plan
1.6/Assemblies/KeyzAllowUtilities.dllandCompatibility/rwmt.Multiplayer/1.6/Assemblies/KeyzAllowUtilities.Multiplayer.dll).[KAU MP] ...log lines from that session.