Skip to content

fix/retail-compat: group flow, boards click, walk-crash CTD, profile-click UX#7

Open
Caeldeth wants to merge 13 commits into
mainfrom
fix/retail-compat
Open

fix/retail-compat: group flow, boards click, walk-crash CTD, profile-click UX#7
Caeldeth wants to merge 13 commits into
mainfrom
fix/retail-compat

Conversation

@Caeldeth
Copy link
Copy Markdown
Collaborator

@Caeldeth Caeldeth commented Apr 30, 2026

Summary

Multiple retail-compat and quality-of-life fixes that landed across the fix/retail-compat branch. Investigation context for each lives in Comhaigne/docs/plans/hybrasyl.client/retail-compat-scoping.md.

Group flow — restored end-to-end on Hybrasyl + retail.

  • Replace Chaos.Networking.DisplayGroupInviteConverter via custom raw 0x63 deserializer in ConnectionManager.HandleDisplayGroupInvite. The upstream converter has its Deserialize/Serialize branches functionally swapped against the actual Hybrasyl/retail wire shape (Invite=1 reads box info that isn't there; ShowGroupBox=4 ignores box info that is) and reads class slots in the wrong order (Monk before Rogue vs. the actual W/Wiz/R/P/M emission). Subtypes 1 (Ask) and 4 (RecruitInfo) populate DisplayGroupInviteArgs correctly so downstream code is unchanged; subtypes 2 (Member) and 5 (RecruitAsk) are logged + dropped pending retail packet capture.
  • Optimistic local toggle for CGroup HUD indicator and StatusBook GroupBtn — Hybrasyl's 0x2F handler doesn't push a SelfProfile back unless the toggle leaves a group, so wait-for-server desyncs the icon. All three click sites (HUD, profile, F4 settings) now route through WorldState.UserOptions.Toggle(12) for sync.
  • Group invite popup ZIndex = 10 so it floats above StatusBook / GroupTabControl / NPC dialogs (matches DisconnectPopup level).

Boards — messageboard signpost click now works on retail (and continues to work on Hybrasyl).

  • New ConnectionManager.ClickFloorTile(x, y) sends the 7-byte coord click [0x43][0x03][u16 x BE][u16 y BE][0x01] with the trailing flag byte set to 1 (floor-aligned anchor convention). Wire shape verified by Comhaigne's 0x43 RE refresh — without the flag byte retail defaults to 0 (above-tile) and silently drops the signpost dispatch.
  • OnRootDoubleClick falls back to ClickFloorTile when GetEntityAtScreen returns null and TileHasForeground is true. Matches retail UX where signposts open on double-click; Hybrasyl single-click QoL keeps the existing 5-byte ClickTile path unchanged.
  • Bypasses sealed ClickArgs/ClickConverter — same pattern as the doors fix's ClickDoor (folds back together post-Phase-3).

Walk-crash CTDAnimationSystem.GetSteppedWalkOffset divided by startX without guarding startX == 0. When StartWalk was called with a non-cardinal direction (item-induced dialog while walking), WalkStartOffset was Vector2.Zero and the next Update frame crashed. Added an early-return guard.

Female-invisible body sprite — two-layer rendering bug for female characters in invisible state.

  • Layer 1 (gender derivation, commit 6f50fc3): WorldState.AddOrUpdateAisling only checked BodySprite.Female/FemaleGhost for gender, missing the FemaleInvis/FemaleHead/BlankFemale cases. Switched to the canonical DataUtilities.DetermineGender helper covering all 13 enum values.
  • Layer 2 (asset-redirect carve-out, commit 895aa0c): with gender now correctly Female, BodySprite.FemaleInvis resolved to wb003 — but wb003 in khanwad contains wrong art (a "guard outfit"), not an invisible body. mb003 is the correct invisible silhouette and is gender-neutral when invisible (no skin shows), so it composites cleanly under w-prefix overlays. Extended the existing "shield always loads from male archive" carve-out at AislingRenderer:1240/921 to also force male archive for the body 'b' layer when spriteId == 3. Both call sites route through a new ForceMaleArchive helper documenting the wb003 hack and naming the revert condition (a .datf asset pack shipping correct wb003 art).

Profile-click UX — reworked single-click on aislings to default EnableProfileClick = true and route Self profile through a clean event chain.

Outbound packet loggingGameClient.Send now logs opcode + length + first 64 hex bytes via NoticeDebugLog, mirroring the existing inbound log. Useful for the next retail-compat investigation.

Test plan

Verified end-to-end on Hybrasyl QA against feature/doors and against retail (da0.kru.com) where applicable:

  • Group invite popup appears with correct sender name when partner sends /group <self> on Hybrasyl
  • Group invite popup floats above StatusBook / NPC dialogs when one is already open
  • Accept-or-cancel routes correctly from popup
  • CGroup HUD indicator icon repaints immediately on click (both grouped and not-grouped states)
  • StatusBook GroupBtn icon repaints immediately on click
  • F4 settings panel option 12 reflects toggle from any of the three click sites
  • Recruit-board flow displays name / note / level range / class composition correctly
  • Messageboard signpost at lod505 (19, 3) opens on single-click against Hybrasyl
  • Messageboard signpost at lod505 (19, 3) opens on double-click against retail
  • F7 mailbox flow regression-checked, unaffected by boards changes
  • Walking through retail item-induced dialog no longer CTDs in GetSteppedWalkOffset
  • Female characters in invisible / head-only / blank states render with female hair/face/equipment overlays
  • Female-invis body silhouette renders as the correct invisible sprite (mb003), not the wb003 guard outfit

Notes for reviewers

  • The three Chaos.Networking bypasses landing here (group 0x63 deserializer, boards ClickFloorTile) are the second and third workarounds in the chaos-networking-removal-direction.md "Tactical workarounds (pre-removal)" log. They fold back together with the doors fix's ClickDoor (PR fix(doors): retail-compat door clicks + tile-resolution Y-comp #6) once Phase 3 owns ClickArgs/DisplayGroupInviteArgs locally.
  • The ForceMaleArchive body-'b' carve-out for spriteId 3 is an asset-bug workaround, not a protocol/architecture change. Revert the spriteId-3 branch in ForceMaleArchive once a .datf asset pack ships correct wb003 art.
  • Outbound logging is "Temporary diagnostic" by NoticeDebugLog's own description but kept in for the next retail-compat investigation. Cheap when the log target isn't enabled.
  • Member=2 and RecruitAsk=5 group subtypes are documented as "log + drop pending retail packet capture" — Hybrasyl never emits these so we have no reference shape; legacy retail must accept some shape but it's never been documented.

🤖 Generated with Claude Code

Comment thread .claude/settings.local.json Outdated
@@ -3,7 +3,15 @@
"allow": [
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.

remove and add .claude to .gitignore?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

testing - done

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

how about now

return;
case 4: //Hybrasyl `RecruitInfo` — full WriteInfo block. Map to Chaos enum ShowGroupBox.
{
var recruiter = reader.ReadString8();
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.

this shit is making me screech (vs a library)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Documented as a section to replace when new networking library is available.

Caeldeth and others added 13 commits May 19, 2026 08:16
Captures the three retail-compatibility bugs targeted by this branch
(N/S doors, group flow, board-click) along with confirmed symptoms,
suspect commits, ground-truth Hybrasyl enum values, test targets
(Rucesion 40,42 + Piet 29,12 + lod505 19,3), and the phase-by-phase
plan with required review gates.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tion

Investigation pass concluded the bug is not a regression on c0237ef /
d74127b / 9bb61a6 — the client's outbound click path is genuinely
axis-agnostic, c0237ef was reverted via the 9d3ed3f upstream merge,
DoorTable covers every doors.md N/S pair, and Hybrasyl's 0x43 handler
is byte-identical across main / develop / feature/doors.

Reframed as missing-behavior likely present since inception:

- Inbound 0x32 handling works (other-player door opens render fine
  and become walkable for this client).
- Outbound click reaches retail but doesn't toggle for N/S; legacy
  DA client opens these same doors on retail.
- Leading hypothesis (per user): retail validates trailing payload
  bytes on 0x43 ClickType=3 that Chaos.Client doesn't emit. Hybrasyl
  reads only the first 5 bytes and silently consumes any tail, so
  Hybrasyl tolerates our short packet; retail does not.
- Ruled out: auto-walk/facing requirement, sotp pathfinder gating,
  DoorTable gaps, feature/doors server changes.

Notes the ClickTile-on-any-foreground divergence as intentional
(ClickTile is being expanded for targeting); do not gate on
interactable detection.

Hands off with three concrete unblock paths: wire capture, prior
RE documentation, or brute-force append experiments.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… evidence

Walk crash (new Phase 4):
- User-supplied stack trace pinpoints DivideByZeroException at
  AnimationSystem.GetSteppedWalkOffset:654 (`y = x * startY / startX`)
  triggered by AdvanceWalk during WorldScreen.Update.
- Root cause: GetWalkOffset returns Vector2.Zero for any non-cardinal
  Direction, leaving startX = 0; subsequent line 654 divides by it.
  The dialog/item just sets walk state with a bad Direction; the
  crash is the next-frame consequence.
- Documents minimal proposed fix (early return when startX == 0)
  with rationale, and follow-up audit of GetWalkOffset's silent
  Vector2.Zero default.

Doors (Phase 1, packet-capture finding):
- E/W door click on retail receives 0x0C CreatureWalk (len 18) +
  0x0E RemoveEntity (len 13). N/S receives nothing.
- Retail's door model is entity-based (invisible door entity on
  the tile, opened by walking it off + removing). Hybrasyl's 0x32
  Door is a different model. HandleDoor (0x32) is dead code on
  the retail path; existing CreatureWalk/RemoveEntity handlers
  cover door-open by accident.
- Confirms the trailing-payload hypothesis: retail's 0x43 handler
  rejects N/S clicks before any 0x0C/0x0E is emitted.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The branch's scoping doc is now canonical in the unified Hybrasyl
document repo. Drop it here so a future merge to main doesn't
resurrect it.

The 24 sibling docs that exist on this branch's tree (because they
predate the doc move on main) will be removed when the branch
eventually merges or rebases onto main — main's deletion takes
precedence in 3-way merge since this branch never modified them.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restructures the double-click-on-entity dispatch in HandleWorldClick
so Aisling clicks (self and other) flow through a single
EnableProfileClick gate rather than treating self as unconditional.

- Self double-click → self-profile (when EnableProfileClick is on).
- Other Aisling double-click → ClickEntity (server replies with
  their profile). Previously gated but no-op for most users since
  the default was off.
- Non-Aisling entity click path is unchanged in behavior.

Default EnableProfileClick flipped to true so other-Aisling profile
viewing works out of the box; users can still disable both
behaviors via the setting.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…onger render as male

WorldState.AddOrUpdateAisling derived Gender from BodySprite via an
inline ternary that only matched BodySprite.Female and FemaleGhost.
The remaining female variants — FemaleInvis, FemaleHead, BlankFemale
— fell through to the default Gender.Male, so any female character
in invisible state (and head/blank states) rendered with male body,
hair, face, and equipment EPF variants.

Switches to the canonical DataUtilities.DetermineGender helper which
already covers all 13 BodySprite enum values correctly. Single source
of truth for gender derivation; no more drift between callers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…w reader

The upstream DisplayGroupInviteConverter has its Deserialize and Serialize
branches functionally swapped against the actual Hybrasyl/retail wire shape:
Invite=1 attempts to read full DisplayGroupBoxInfo (which Hybrasyl never
emits), while ShowGroupBox=4 ignores the box info Hybrasyl always carries.
The class-slot order in Deserialize is also wrong (Monk before Rogue).
Result: group invite popups never appeared on Hybrasyl.

HandleDisplayGroupInvite now bypasses Client.Deserialize and reads the
wire directly via SpanReader, dispatching on the raw subtype byte. Subtypes
1 and 4 populate DisplayGroupInviteArgs with the correct shape; subtypes
2 (Member) and 5 (RecruitAsk) log + drop pending retail packet capture.

See chaos-networking-removal-direction.md "Tactical workarounds" for
the bypass log and post-Phase-3 fold-back plan.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…bove panels

Three visual fixes for the group flow:

- Group invite popup ZIndex raised to 10 so it floats above StatusBook,
  GroupTabControl, NPC dialogs, etc. Matches DisconnectPopup's level.
- HUD CGroup indicator and StatusBook GroupBtn now route through
  WorldState.UserOptions.Toggle(12) so all three toggle paths (HUD, profile,
  F4 settings) stay in sync.
- The case-12 SettingToggled handler now optimistically updates WorldHud
  and StatusBook visuals on click before sending 0x2F. Hybrasyl's 0x2F
  handler doesn't push a SelfProfile back unless the toggle actually leaves
  a group, so wait-for-server visual updates desync against the server's
  flag flip. Subsequent SelfProfile updates reconcile if state diverges.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ConnectionManager already writes inbound packets to NoticeDebugLog
("inbound opcode=0xNN len=… handled=…"); the symmetric outbound
side was missing, so during retail-compat investigation we couldn't
distinguish "client never sent the packet" from "client sent but
retail ignored it."

Logs opcode, length, and the first 64 bytes of payload (truncated
to keep the log readable for long packets). Logging happens BEFORE
encryption so the hex matches what wire-format docs/RE describe —
the Crypto.ClientEncrypt step would otherwise scramble the bytes.

This is "Temporary diagnostic" by NoticeDebugLog's own description;
remove or gate the call when the door retail-compat work concludes.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…posts

Retail's 0x43 type=3 coord-click handler validates a trailing flag byte
that distinguishes floor-aligned sprites (signposts, frames, ground items)
from above-tile sprites (door panels, awnings). The existing 5-byte
ClickTile sends no flag, retail defaults to 0 ("above-tile click"),
and the signpost dispatch silently drops because signposts are
floor-aligned. Hybrasyl ignores the trailing byte entirely and looks
up Map.Signposts[(x,y)] directly, which is why single-click works
on Hybrasyl but spam-clicking does nothing on retail.

Wire shape verified by Comhaigne's 0x43 RE refresh (commit 633063a):
  [0x43][0x03][u16 x BE][u16 y BE][u8 flag]
flag = 1 for floor-aligned sprites, 0 for above-tile sprites. The flag
is the WorldObject_Static +0x80 anchor convention (inverted) on the
clicked sprite.

Adds ClickFloorTile(x, y) in ConnectionManager that bypasses the sealed
ClickArgs/ClickConverter — same pattern as the doors fix's ClickDoor.
Wires OnRootDoubleClick to fall back to ClickFloorTile when no entity
is hit and the tile has foreground; matches retail's UX where signposts
open on double-click. Hybrasyl's single-click QoL keeps the existing
5-byte ClickTile path unchanged.

Smoke-tested 2026-04-29 on lod505 (19, 3) against both Hybrasyl
(single-click) and retail (double-click) — board opens correctly.
F7 mail flow regression-checked, unaffected.

See chaos-networking-removal-direction.md "Tactical workarounds
(pre-removal)" for the third entry in the Chaos.Networking bypass
log (after doors, group).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…rong art)

Commit 6f50fc3 fixed gender derivation so BodySprite.FemaleInvis
correctly resolved to Gender.Female, but exposed a separate asset
bug: wb003 in khanwad is not an invisible body sprite — it contains
unrelated "guard outfit" art. So female-invis characters rendered
female hair/face/equipment overlays on top of guard-outfit body.

mb003 is the correct invisible silhouette and is gender-neutral when
invisible (no skin shows), so it composites cleanly under w-prefix
overlays. Extends the existing "shield always loads from male
archive" carve-out at AislingRenderer:1240/921 to also force male
archive for the body 'b' layer when spriteId == 3.

Both call sites now route through a new ForceMaleArchive helper so
the wb003 hack is documented in exactly one place, with an explicit
revert condition: remove the spriteId == 3 branch if/when a .datf
asset pack ships correct wb003 art.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
main's .gitignore (line 403, via 6e504d6) already has the broad `.claude`
rule, but the file remained tracked from before that rule was added.
Untrack it so the ignore actually takes effect on this branch and matches
the convention on main / feature/item-asset-packs.

Addresses review comment from baughj on PR #7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…MOVE WHEN

Tightens the in-code workaround marker on the raw 0x63 deserializer to match
the bar set by ClickFloorTile / ForceMaleArchive:

- Promotes the inline `//` block to a proper XML <summary> + <remarks>.
- Adds an explicit TACTICAL WORKAROUND label and a REMOVE WHEN clause tying
  replacement to the new Hybrasyl networking library owning DisplayGroupInviteArgs
  locally (per chaos-networking-removal-direction.md).
- Cross-references the other two pre-removal bypasses (ClickFloorTile, ClickDoor)
  so a future reader sees the full set.
- Strengthens the subtype 2 / 5 callouts with TODO markers so they grep easily.

Comment-only — no behavior change.

Addresses review comment from baughj on PR #7.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Caeldeth Caeldeth force-pushed the fix/retail-compat branch from e5fa6d9 to 58f5392 Compare May 19, 2026 13:21
@Caeldeth Caeldeth requested a review from baughj May 19, 2026 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants