Illusion Temple Mini Game - Event

[bulgarashi]

Illusion Temple mini-game event

Implements the full Illusion Temple event: a team-based PvP mini game where two teams (Allied Forces / Illusion Forces) fight over a sacred relic held by a randomly-spawning Stone Statue.

What's included

• Core game loop: statue → relic pickup → carry → deliver to own team's storage → score. Relic drops on death or on leaving the event and can be picked up by anyone. A team needs at least 2 points, and more than the opponent, to be declared the winner (a 1:0 finish is a draw, matching the original event).
• Teams & state: players are split into two teams on start, each gets a live per-player state update (own team's positions, current relic carrier, remaining time) and an end-of-game result screen.
• Rewards: experience is granted automatically to the winning team; other reward types (e.g. item drops) are only granted once the player explicitly claims them by closing the result dialog, matching the original "click Close to be compensated" flow.
• Skill points & special skills: killing an enemy player (+1) or a roaming arena monster (+2) builds a skill-point pool (start 10, cap 90) that fuels four event-only skills (210 Order of Protection, 211 Restraint, 212 Tracking, 213 Weaken), each costing 10 points.
• Admin-configurable minimum player count (EF migration included) instead of hardcoded per event.
• Corrected packet layouts for IllusionTempleState/IllusionTempleResult (missing field, wrong offset, and a padding gap that the original client's C struct expects) — new packets for event state, holy-relic notification, skill usage/points, and reward requests.
• Scheduling: starts every 2 hours (matching the official Webzen schedule), plus a GM chat command to force a start for testing.
• Map data fixes: corrected statue/guardian/relic-box spawn positions on all six temple maps (verified against a working Season 6 Episode 3 server), added the 32 roaming arena-monster spawns (temple 6 has none), and fixed each temple's safezone map (previously defaulted to itself, so a player warped to "safezone" was sent right back into the arena).
• Update plugin so existing databases get all of the above without a full reinstall (including a fix for a ticket-item number swap).
• Test coverage for entrance timing, minimum-player handling, team assignment (even/odd counts), the statue/relic loop, scoring, and the reward flow.

Known issues

• On the end-of-game result screen, the displayed character class is wrong when tested with a 1.04d client, because that client's class IDs don't match Webzen's numbering. Needs a client-version-aware class ID mapping in the result packet.

[sven-n]

Review

Substantial, well-documented feature work — the comments explain why (the client's C-struct padding, the hardcoded arena barriers, the score-table-before-warp ordering) rather than restating the code, and the packet-layout corrections look right. CI is green (Codacy is action_required, not a failure). But there are several real bugs, one of which makes a headline feature of the PR a no-op.

Blocking

1. UpdateVersion.IllusionTempleData = 100 collides with AddCastleSiegeData = 100 — src/Persistence/Initialization/Updates/UpdateVersion.cs:530

IConfigurationUpdatePlugIn documents the version as "must be unique over all DataInitializationKeys", and it's used two ways, both of which break:

• Key => (int)Version (UpdatePlugInBase.cs:16) keys the strategy provider. StrategyPlugInProvider.ActivatePlugIn (StrategyPlugInProvider.cs:79-86) keeps the first registration and logs "will not be effective" for the second, and AvailableStrategies returns only the effective ones — so either the castle siege update or the illusion temple update disappears from the update list entirely.
• DetermineAvailableUpdatesAsync filters with !installedUpdates.Contains(up.Version) (DataUpdateService.cs:59). Any existing DB that installed update 100 already considers this update installed.

So "existing databases get all of the above without a full reinstall" doesn't actually happen. Should be 104. The XML doc is also missing its opening <summary> tag.

2. Re-entrant warp in OnObjectRemovedFromMapAsync — IllusionTempleContext.cs:700-732

GameMap.RemoveAsync awaits ObjectRemoved inline (GameMap.cs:177-180), and Player.WarpToAsync calls TryRemoveFromCurrentMapAsync → map.RemoveAsync before PlaceAtGateAsync (Player.cs:880-889). So the handler's WarpToAsync(devias) runs nested inside whatever warp removed the player:

MovePlayersToSafezoneAsync → WarpToSafezoneAsync → RemoveAsync
   └─ OnObjectRemovedFromMapAsync → WarpToAsync(Devias) → PlaceAtGate(Devias), MapChange
   ← returns
PlaceAtGate(safezone gate), MapChange     ← player now on two maps, two MapChange packets

This fires on the normal end-of-game path, on ClaimRewardAsync, and on any other warp off the temple map. It also silently overrides the per-temple SafezoneMapNumber this PR just fixed. Suggestion: limit the handler to state cleanup (relic drop, _teams/_skillPoints removal, party kick) and leave the warping to ClaimRewardAsync / MovePlayersToSafezoneAsync.

3. Every failed entry is reported to the client as success — RemoteView/MiniGames/Extensions.cs:84-87

public static byte ToIllusionTempleEnterResult(this EnterResult enterResult)
{
    return 0;
}

The parameter is ignored and the remark itself says failures should be 1. ShowMiniGameEnterResultViewPlugIn sends this for every refusal.

4. Item rewards are silently lost for most winners — IllusionTempleContext.cs:844-869

ClaimRewardAsync defers non-experience rewards to DoesRewardApply, which for WinnerOrInWinningParty requires Winner.Party == player.Party. But each departing winner goes through KickMySelfAsync → Party.ExitPartyAsync, which disposes the party once fewer than 2 members remain (Party.cs:417-436), nulling everyone's Party. After that only the single arbitrary player returned by Winner (a FirstOrDefault over a ConcurrentDictionary, :229) still qualifies. On a 2v2 the second winner gets nothing; on larger teams most of them get nothing. Experience is unaffected, since it's granted eagerly in GameEndedAsync.

5. The 20s preparation delay runs inside OnGameStartAsync — :584-593

The tests drive OnGameStartAsync directly (IllusionTempleContextTest.cs:376-379), so ~15 tests each block for the full PreparationDuration, adding roughly five minutes to the suite. Make it a protected virtual TimeSpan the tests can shorten, or move the preparation phase out of the start hook.

Correctness

6. TeleportToStartCoordinatesAsync doesn't do what its comment says — :919-933. cordinatesAlliedForces is a local copy of a readonly field, so += new Point(1, 0) is discarded every iteration and every member of a team is moved to the same tile. Either spread by index or drop the += and the // every player on differend point comment (also a typo). The two fields don't follow the _camelCase convention used elsewhere in the file, and duplicate the literals in GetSpawnGate.

7. player.Party = null; — :548. The only other assignments to Party are inside Party itself; this bypasses KickMySelfAsync and leaves the player in the old party's _partyMembers array. EnterMiniGameAction already kicks non-party mini-game entrants properly (EnterMiniGameAction.cs:125-128), so the line is redundant at best.

8. Special skills hit your own team. UseRestraintAsync (:465) and UseWeakenAsync (:508) only exclude target == player — no team check, so you can freeze a team mate for 15s. UseSkillAsync also doesn't check IsEventRunning or that the caster is alive.

9. Reported experience is wrong for ExperiencePerRemainingSeconds — :899. _grantedExperience sums RewardAmount, but GiveRewardAsync grants seconds * RewardAmount (MiniGameContext.cs:573-580); AddExperienceAsync also applies server rates. The seeded reward is plain Experience, so this only bites custom configs — but the score board will lie.

10. Relic race and item leak — :272-304. _relicCarrier is checked and set with awaits in between (same in OnPlayerPickedUpItemAsync), so two players talking to the same statue can both walk away with a relic. Separately, the Item is created via CreateNew<Item>() before the inventory-space check, so a full inventory leaves an orphan entity in the persistence context.

11. The relic is never reclaimed at game end. A carrier who never delivers keeps "Cursed Castle Water" in his inventory permanently.

12. Dead paths. IllusionTempleEventStatus.Ended and WaitingRoom are never sent — the client's event UI is never told the match is over. IIllusionTempleSkillEndedViewPlugin has a remote-view implementation but no caller, so the special-skill effects never notify the client when they expire.

13. Hardcoded Devias exit gate duplicated in ClaimRewardAsync (:860-868) and OnObjectRemovedFromMapAsync (:711-719), rather than the map's configured safezone — which is what the SafezoneMapNumber map fixes in this same PR exist for.

Consistency / style

• EnterMiniGameAction.ShowRefusalAsync: hardcoded English strings where the codebase (and this PR) uses localized PlayerMessage resources; several needless $""; "Killers can`t enter!" uses a backtick. These messages now fire for all mini games, which is a behavior change beyond the PR's stated scope.
• Copy-paste docs: StartIllusionTempleEventChatCommandPlugIn — "handles the startcc command"; IllusionTempleGameServerState — "for a chaos castle event"; MiniGameContext.GetSpawnGate — "Gets spown gate" with empty <param>/<returns>.
• StartIllusionTempleEventChatCommandPlugIn.HandleCommandAsync is async with no await (CS1998) and gives the GM no feedback when the plugin isn't registered.
• IllusionTempleStartPlugin uses literal [Display] strings while its siblings use PlugInResources; the two new sub-packet handlers lack [Display] entirely.
• File/type name mismatches: IllusionTempleEnterHandlerPlugin.cs → …PlugIn (its copyright header names a file that doesn't exist), IllusionTempleUserCountViewPlugin.cs → …PlugIn, and IllusionTempleStartPlugin breaks the PlugIn suffix convention.
• TalkNpcAction: cases 383/384 are byte-identical (collapse them), case bodies aren't indented, and both carry the same misleading comment. TalkToNpcTeamStorageAsync's body is indented one level too deep.
• Member ordering: IllusionTempleScore.MinimumWinningScore sits between properties; SorcererSpiritPositions is declared after a method in all six map files.
• IllusionTempleTeam.cs lives in MiniGames/ but declares namespace MUnique.OpenMU.GameLogic.
• Unrelated whitespace noise in MiniGameContext.cs:37,91 (trailing whitespace + a stray blank line) and an extra blank line in Player.cs:1550.
• GameContextTestHelper now sets MaximumLevel = 400 for every test using the helper — a global change tucked into a feature PR.
• Group == 14 && Number == 64 is repeated three times in the context; extract constants.
• The arena-monster stat table and map spawn coordinates are duplicated verbatim between NpcInitialization.CreateIllusionSorcererSpirits and IllusionTempleDataUpdatePlugIn. Worth a shared source given how easily these drift.
• FixTicketItemNumbers changes Number but not the deterministic SetGuid(group, number) id — deliberate (changing it would break owned-item FKs), but it means an updated DB's "Illusion Sorcerer Covenant" keeps the id of (13, 50) while a fresh install gets (13, 51). That divergence should at least be documented, since later updates that resolve items by Guid will hit it.
• Tests reach into private/protected members by reflection (pragmatic, given the constructor starts a real-time countdown, but brittle), and FinishesWhenTooFewPlayersRemainAsync asserts only PlayerCount < 2 — the precondition it just created, not that the event finished.

Questions

• The old Cursed Statue / Captured Stone Statue spawns (NPCs 658–668) are deleted from all six maps. Fresh installs are fine, but existing DBs only get the replacement via the update plugin — which currently can't run (see Bug at player appearance - weird wing is shown #1). Intended?
• The known issue about wrong class IDs on 1.04d clients is fine to defer, but IllusionTempleResult has no client-version handling at all right now, so it will be wrong for every client that doesn't use Webzen's numbering.

---

Generated by Claude Code

[sven-n]

Follow-up review (head cef63b0e)

Re-reviewed after the three fix commits, plus a pass over the parts I hadn't covered the first time. Nearly everything from the previous round is fixed, and fixed at the right level rather than patched over. The Azure build is green on this head (Codacy is action_required, i.e. a gate that didn't run, not a failure).

Verified fixed

Previous finding	Fix
UpdateVersion collision with AddCastleSiegeData	Now IllusionTempleData = 112; I checked the whole enum for duplicate values, there are none.
Re-entrant warp in OnObjectRemovedFromMapAsync	Both WarpToAsync(Devias) calls removed, with a remark explaining why the handler must not warp. ClaimRewardAsync uses WarpToSafezoneAsync(), so the per-temple safezone fix actually takes effect.
Enter result always reported success	enterResult == EnterResult.Success ? 0 : 1.
Item rewards lost once the winning party is disposed	Solved properly with the new IsWinner / IsInWinningParty / IsWinnerOrInWinningParty virtuals. I checked the DoesRewardApply rewrite against the original boolean expressions for Winner, Loser, WinningParty and WinnerOrInWinningParty — all four are logically equivalent, so Blood Castle / Chaos Castle / Devil Square behaviour is unchanged.
20s preparation delay in every test	PreparationDuration is settable and zeroed in CreateContextAsync.
All team members on one tile	i / 2 as the in-team index; the max offset (+4) stays inside both spawn rectangles.
player.Party = null	Removed.
Friendly fire on special skills	IsHostileTarget, plus IsEventRunning and IsAlive checks in UseSkillAsync.
Wrong reported experience	ExperiencePerRemainingSeconds now multiplied by the remaining seconds.
Relic race / orphan item entity	Interlocked.CompareExchange claims the carrier slot before the first await, with a try/catch that hands the claim back; the item is only created after the space check and DeleteAsync'd if it can't be added.
Relic kept forever	RemoveRelicFromInventoryAsync on claim, plus the existing drop path for the carrier.
Ended / skill-ended never sent	Both wired up.
Hardcoded Devias gates	Gone.

Plus the localized PlayerMessage resources, the copy-paste doc strings, startcc→startit, GM feedback when the plugin is missing, the TalkNpcAction case collapse, the IllusionTempleTeam namespace, and MaximumLevel becoming an opt-in test parameter instead of a global default. FinishesWhenTooFewPlayersRemainAsync now asserts the state actually leaves Playing.

Two good catches of your own along the way: span.Clear() before writing the result packet (those three alignment bytes per entry were never written, so stale pipe-buffer content went out on the wire), and the try/finally around the entry dialog so a failure can't strand the player in NpcDialogOpened.

Open: character class conversion is wrong for three classes

RemoteView/MiniGames/Extensions.cs:99-106. The remark documents the upper nibble as 0 base, 2 second class, 3 master. That holds for the lines with three tiers, but Magic Gladiator, Dark Lord and Rage Fighter have only two, and their master classes sit at internal % 4 == 1 (all three are in CharacterClassNumber.AllMasters):

Class	Internal	Produced	Expected
Grand Master	3	0x30	0x30 ✅
Blade Master	7	0x31	0x31 ✅
High Elf	11	0x32	0x32 ✅
Dimension Master	23	0x35	0x35 ✅
Duel Master	13	0x13	0x33 ❌
Lord Emperor	17	0x14	0x34 ❌
Fist Master	25	0x16	0x36 ❌

Evolution nibble 1 isn't a value the scheme defines, so those three will still render wrong on the score board — the same bug the commit set out to fix, for the three classes that weren't in the test sample. var evolution = step == 1 ? 3 : step; covers it, or an explicit map. A unit test over all 18 CharacterClassNumber values would be worth adding while you're in there.

Open: ordering of Ended vs. the result packet

IllusionTempleContext.cs:958-962 sends IllusionTempleEventStatus.Ended to everyone before base.GameEndedAsync shows the score table. The packet documentation for IllusionTempleEventState says Ended means the client closes the event interface — if that also tears down the result dialog, the client would never send the reward request that ClaimRewardAsync depends on. If you've already seen the result screen work on a live client with this order, ignore this; otherwise sending Ended after the score table (or at exit) is the safer order.

New: the targeted special skills probably can't resolve their target

IllusionTempleSkillRequest is 8 bytes: header, ShortBigEndian SkillNumber at 4-5, then bytes 6 and 7, which the definition models as Byte TargetObjectIndex + Byte Distance. Every other client-to-server skill packet in the same file encodes the target as a two-byte big-endian id right after the skill id:

• TargetedSkill: ShortBigEndian SkillId, ShortBigEndian TargetId
• TargetedSkill075 / TargetedSkill095: Byte SkillIndex, ShortBigEndian TargetId
• AreaSkill: ShortBigEndian SkillId, …, ShortBigEndian ExtraTargetId

Eight bytes is exactly header + short skill + short target, with nothing left over for a distance byte. If the field really is a big-endian short, IllusionTempleSkillRequestHandlerPlugIn.cs:41 reads only its high byte, so CurrentMap.GetObject(targetObjectIndex) resolves the wrong object (or none, for any id below 256) — and Restraint (211) and Weaken (213), the only targeted skills, would silently fail through IsHostileTarget. UseSkillAsync already takes a ushort for this parameter, which suggests two bytes were the intent; the byte just widens silently at the call site.

The definition predates this PR (ClientToServer is untouched here, and its siblings still carry <SentWhen>?</SentWhen> placeholders), but this PR is its first consumer. One capture of a Restraint cast would settle it: if it's a big-endian short, change the XML field type and drop Distance — the handler then needs no change at all.

Checked and clean

• Spawn data consistency. Statue pool, both guardians and both storage boxes match exactly between all six map initializers and the update plugin's constants. The 32 arena-monster positions are byte-identical between the update plugin and maps 1-5, and map 6 correctly has none.
• No 0xBF sub-code collision. The three new handlers use 0x00, 0x02, 0x05; the only other group members are 0x17 (market place) and 0x51 (MU Helper), so none is silently dropped by StrategyPlugInProvider — the failure mode that bit the update version.
• EF migration. One non-nullable integer with defaultValue: 0, correct Down, snapshot matches, and it's still the newest migration by timestamp after the master merge.
• Name-carrying packets don't need clearing. ByteSpanExtensions.WriteString calls target.Clear() before encoding, so IllusionTempleHolyItemRelics zero-fills its name bytes; the 10-character name cap also rules out an overflow there.
• The remaining view plugins (SkillEnded, SkillPointUpdate, SkillUsageResult, HolyItemRelics) all null-check the connection and follow the codebase write idiom. Packet sizes stay well under the C1 length limit at the 10-player cap (the result packet tops out at 207 bytes).

Minor / carried over

• IllusionTempleHolyItemRelics declares <Length>16</Length> but its Name field has none, so the generator emits both a fixed Length and a GetRequiredSize(string), with the getter reading _data.Length - 6. It works because the plugin sizes the span at exactly 16, but adding <Length>10</Length> would make it explicit — the same fix applied to PlayerResult.Name. Its <SentWhen>?</SentWhen> / <CausedReaction>?.</CausedReaction> placeholders are fillable now that the packet has a real implementation.
• UpdateVersion.cs:567 has trailing whitespace. (The other git diff --check hits are all in generated files.)
• OnPlayerPickedUpItemAsync still does a plain check-then-set on _relicCarrier while TalkToNpcStoneStatueAsync now uses CompareExchange. Only one relic is ever in play, so it's near-unreachable, but the asymmetry invites a future bug.
• NotifyWhenSkillEffectEnds subscribes before AddEffectAsync; if the effect list immediately replaces and disposes the effect, the client gets an instant "ended". Cosmetic.
• Still open, all cosmetic: IllusionTempleEnterHandlerPlugin.cs / IllusionTempleUserCountViewPlugin.cs filenames vs. their …PlugIn type names; no [Display] on the reward and skill request handlers; alliedForcesCoordinates / illusionForcesCoordinates not _camelCase, and duplicating the rectangles in GetSpawnGate; MinimumWinningScore declared between properties; SorcererSpiritPositions after a method in all six map files; WaitingRoom never sent; the arena-monster stat table and spawn coordinates duplicated between NpcInitialization and the update plugin (currently in sync, as noted above); FixTicketItemNumbers leaving the item's deterministic Guid at (13, 50); no bounds check on request.ItemSlot - EquippableSlotsCount before the (byte) cast.

---

Generated by Claude Code

[didiconcs]

Thanks for this - the Illusion Temple event has been an open issue with zero progress since 2023, so a full implementation is a big contribution. I pulled feature/illusion-temple-mini-game into an isolated worktree and gave it a build + read-through.

Build & tests

• dotnet build src/Startup/MUnique.OpenMU.Startup.csproj -c Debug after building the persistence source generator first: 0 errors, 216 warnings (all pre-existing StyleCop/analyzer patterns across the solution, not introduced here - the only warnings actually inside the new files are a couple of cosmetic ones, noted below).
• dotnet test tests/MUnique.OpenMU.Tests --filter FullyQualifiedName~IllusionTempleContextTest: 16/16 passing, covering entrance gating, even/odd team split, the statue -> carry -> deliver -> score loop, wrong-storage delivery being a no-op, death dropping the relic and re-pickup by another player, the deferred item-reward claim flow, and the "finish early on too few players" path.
• Verified the win condition against the PR description ("at least 2 points, and more than the opponent"): IllusionTempleScore.LeadingTeam implements exactly that (>= MinimumWinningScore && > other team), so a 1:0 finish correctly falls through to null (draw). Matches.

A few things worth a look

1. IllusionTempleRewardRequestHandlerPlugIn and IllusionTempleSkillRequestHandlerPlugIn are missing the [Display(Name = ..., Description = ..., ResourceType = typeof(PlugInResources))] attribute that every other ISubPacketHandlerPlugIn in the codebase carries - including your own IllusionTempleEnterHandlerPlugIn a few lines away. Without it these two won't get a readable name/description in the admin panel's plugin list (just the bare type name). Small fix, but for consistency with the rest of GameServer/MessageHandler.

2. src/GameServer/MessageHandler/MiniGames/IllusionTempleEnterHandlerPlugin.cs - the file name spells "Plugin" (lowercase i) while the class and copyright header both use "PlugIn". That's what's causing the SA1638 warning in the build output. Your two sibling files in the same folder (IllusionTempleRewardRequestHandlerPlugIn.cs, IllusionTempleSkillRequestHandlerPlugIn.cs) already use the correct casing, so this one's just a rename away from matching.

3. IllusionTempleContext.TalkToNpcTeamStorageAsync (lines ~362-419) is indented one extra level (12 spaces instead of 8) for its whole body - stands out since the rest of the file is very clean. Cosmetic only.

4. Possible race, would appreciate your read on it: the relic-carrier claim in TalkToNpcStoneStatueAsync is done with Interlocked.CompareExchange specifically so two players talking to the statue at once can't both become the carrier - the comment there explains why. But the paths that clear _relicCarrier afterwards (TalkToNpcTeamStorageAsync, DropRelicIfCarriedByAsync, OnItemDroppedOnMap, OnPlayerPickedUpItemAsync, RemoveRelicFromInventoryAsync) all do a plain if (player != this._relicCarrier) return; followed later by a plain assignment, with awaits in between. A player's own delivery to the storage NPC runs on that player's own connection pipeline, while a killing blow against him is processed on his killer's pipeline and reaches OnPlayerDied -> DropRelicIfCarriedByAsync independently - so it looks like the carrier dying at (almost) the same instant he hands the relic in could let both paths pass the != _relicCarrier check and both try to RemoveItemAsync the same relic item. I couldn't reproduce this in the time I had (it's a narrow window and I didn't want to fight the timing artificially), so flagging it as a question rather than a confirmed bug - given how deliberately the pickup race is handled, I'd guess you've already thought about whether the delivery/death race is actually reachable given how actions get serialized elsewhere in the engine.

5. Minor duplication: IllusionTempleInitializer.CreateSpecialSkillEffects() and IllusionTempleDataUpdatePlugIn.AddSpecialSkillEffects() build the same two magic effects with near-identical code. I get why - the update plugin can't call the initializer's private method without also re-running Initialize() and duplicating MiniGameDefinitions on an existing DB - but a small shared internal helper would remove the duplication and one future source of drift between the fresh-install and upgrade paths.

What stood out positively

• The packet layout fixes are well justified, not just asserted - the PlayerResult padding-gap fix in ServerToClientPackets.xml has an in-code comment explaining the client's 20-byte stride and why a plain 17-byte struct silently misreads every entry after the first. Docs, XML and the generated code all agree.
• The known 1.04d class-ID mismatch is disclosed up front in the PR description and the code itself documents the nibble-swap reasoning in Extensions.ToIllusionTempleCharacterClass rather than silently getting it wrong.
• IllusionTempleDataUpdatePlugIn is properly defensive for upgrading a live DB - idempotent existence checks before adding items/monsters/spawns, the ticket item number swap fix, and the safezone-map fix (temples used to default their safezone to themselves, so leaving the event just sent you back into the arena) are all called out with clear comments explaining the underlying bug.

Nice work overall - happy to take another look once the above is addressed, particularly #4 since it's the only one that isn't purely cosmetic.