From dbec4aed8932b043e7e44c3ecdf35453f94daf76 Mon Sep 17 00:00:00 2001 From: pjerra <71784785+pjerra@users.noreply.github.com> Date: Fri, 23 Oct 2026 03:24:24 +0200 Subject: [PATCH 1/4] GM lines wait for a selection echo that cannot answer an older select (#296) selectConfirmed accepted any echo of the player's target field that showed the unit, so an echo from an earlier select of the same unit, with another unit's select still pending behind it, confirmed the new select; the GM line then ran on that other unit. The session now keeps a log of the selections it sent and moves a lower bound on the server's progress with each echo (to the oldest send the echo can answer). A selection is confirmed only when every send from there on is that unit. Reselecting the unit already confirmed needs no echo (the core sends none for an unchanged field). When a new echo could not be told from an old one, a neutral selection (none, else the agent itself) is made and confirmed first. Respawn's wait for a cleared selection uses the same rule. --- README.md | 2 +- cmd/turtle-agent/commander_methods.go | 80 ++++++---- cmd/turtle-agent/dungeon_leader_pulltest.go | 25 +--- cmd/turtle-agent/select_confirm_test.go | 148 ++++++++++++++++++- internal/session/actions.go | 2 + internal/session/chat.go | 10 +- internal/session/selection.go | 155 ++++++++++++++++++++ internal/session/session.go | 36 ++--- internal/session/world.go | 1 + 9 files changed, 375 insertions(+), 84 deletions(-) create mode 100644 internal/session/selection.go diff --git a/README.md b/README.md index 0ffd73f4..59c549ae 100644 --- a/README.md +++ b/README.md @@ -2011,7 +2011,7 @@ part before the bind in one go, then the kit check and the enter phase run as be (a bot that carries the item already gets none and nothing is booked), selects the bot by GUID (the agent's own group or view for a hired bot, the roster for a named one; the selection is confirmed by the server first and the line goes out as one session operation, like `.instance unbind all`), sends **`.additem `** and reads `.bot inv` again until the item shows (at most 8 s, else the run fails saying the line may have run on another player or the bags are full). `lead` is the lead bot, `carrier` the run's `carrier` option, else the bot the object steps use (the first bot that is not the lead, tank or healer). It **refuses without a GM account of level 3** (`.additem` is a developer's command; the account level is read with `.account`, and the run stops before it sends or binds anything). The line is a setup shortcut: `additem 9240 on (test setup: Mallet of Zul'Farrak)` in `setup_shortcuts`, - so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited, and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below. + so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited (an echo that could answer an older select of the unit, with another unit's select sent after it, does not count; when a new echo could not be told from such an old one, the selection is first cleared and that confirmed; reselecting the unit already confirmed needs no echo), and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below. Every GM line the step sends is a **setup shortcut**: it is in `setup_shortcuts` (for example `.tele name Lcwarra uldaman (prepare fresh)`, `.instance unbind all on Lcwarra (prepare fresh)`, `gm visible off`) and is a `gm_shortcut` event of kind `setup` in the trace, so `trace-score`'s diff --git a/cmd/turtle-agent/commander_methods.go b/cmd/turtle-agent/commander_methods.go index 41429b80..5f314986 100644 --- a/cmd/turtle-agent/commander_methods.go +++ b/cmd/turtle-agent/commander_methods.go @@ -376,29 +376,56 @@ func (g remoteGame) GMVisible(ctx context.Context, on bool) error { // gmSelectConfirmWait is how long the server is given to show a selection before a line that acts on it is given up on. var gmSelectConfirmWait = 3 * time.Second -// selectConfirmed selects the unit and waits until the SERVER shows it as the agent's selection (the target field of the agent's own object, -// which the core sends back after it handled CMSG_SET_SELECTION). The core queues that packet (map thread) apart from a GM's chat line (world -// thread), so a line sent right after can run on the selection from before: none (the sender itself) or a stale one, which can be another -// player. Run 6b: `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra. An error, with nothing sent, when -// the server does not show the selection in time. +// selectConfirmed selects the unit and waits until the SERVER is known to have it in force as the agent's selection +// (session.SelectionConfirmed: the target field of the agent's own object, which the core sends back after it handled CMSG_SET_SELECTION, +// in an echo that cannot be an older one). The core queues that packet (map thread) apart from a GM's chat line (world thread), so a line +// sent right after can run on the selection from before: none (the sender itself) or a stale one, which can be another player. Run 6b: +// `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra. Issue #296: an echo still showing the unit from +// an earlier select, with another unit's select pending behind it, is not a confirmation. When an echo of the new select could not be told +// from such an older one (the unit was selected before another unit, and the server has not shown how far it got), a neutral selection +// (none, or the agent itself) is made and confirmed first. An error, with nothing sent, when the server does not show the selection in time. func (g remoteGame) selectConfirmed(ctx context.Context, guid packet.GUID) error { s := g.c.cur() - if _, err := s.Target(ctx, guid, ""); err != nil { - return err - } timer := time.NewTimer(gmSelectConfirmWait) defer timer.Stop() - tick := time.NewTicker(25 * time.Millisecond) + if s.SelectionAmbiguous(guid) { + if n, ok := s.SelectionNeutral(); ok { + if err := selectUnit(ctx, s, n); err != nil { + return err + } + if err := awaitSelection(ctx, s, n, timer.C); err != nil { + return fmt.Errorf("%w (the neutral selection made before %v)", err, guid) + } + } + } + if err := selectUnit(ctx, s, guid); err != nil { + return err + } + return awaitSelection(ctx, s, guid, timer.C) +} + +// selectUnit sends the selection of the unit, or of none for 0. +func selectUnit(ctx context.Context, s *session.Session, guid packet.GUID) error { + if guid == 0 { + return s.ClearTarget(ctx) + } + _, err := s.Target(ctx, guid, "") + return err +} + +// awaitSelection waits until the session says the server has the unit in force as the agent's selection, or until expired fires. +func awaitSelection(ctx context.Context, s *session.Session, guid packet.GUID, expired <-chan time.Time) error { + tick := time.NewTicker(10 * time.Millisecond) defer tick.Stop() for { - sel, ok := s.ServerSelection() - if ok && sel == guid { + if s.SelectionConfirmed(guid) { return nil } select { case <-ctx.Done(): return ctx.Err() - case <-timer.C: + case <-expired: + sel, _ := s.ServerSelection() return fmt.Errorf("the server did not show %v as the agent's selection within %s (it shows %v): not sending a line that acts on the selection", guid, gmSelectConfirmWait, sel) case <-tick.C: } @@ -567,26 +594,15 @@ func (g remoteGame) GoXYZ(ctx context.Context, mapID uint32, at dungeon.Point) e // around the commander. The selection is cleared first and the server is waited for to show it cleared, else the line would act on that // one unit alone (or fail on a player). func (g remoteGame) Respawn(ctx context.Context) error { - s := g.c.cur() - if err := s.ClearTarget(ctx); err != nil { - return err - } - timer := time.NewTimer(gmSelectConfirmWait) - defer timer.Stop() - tick := time.NewTicker(25 * time.Millisecond) - defer tick.Stop() - for { - if sel, ok := s.ServerSelection(); ok && sel == 0 { - break - } - select { - case <-ctx.Done(): - return ctx.Err() - case <-timer.C: - sel, _ := s.ServerSelection() - return fmt.Errorf("the server did not show the selection cleared within %s (it shows %v): not sending .respawn", gmSelectConfirmWait, sel) - case <-tick.C: - } + return respawnUnselected(ctx, g.c.cur()) +} + +// respawnUnselected clears the selection, waits until the server is known to have it cleared (selectConfirmed with no unit, so a stale echo +// of an earlier clear does not count), and sends ".respawn". +func respawnUnselected(ctx context.Context, s *session.Session) error { + g := remoteGame{c: &commanderService{cur: func() *session.Session { return s }}} + if err := g.selectConfirmed(ctx, 0); err != nil { + return fmt.Errorf("the selection was not cleared, not sending .respawn: %w", err) } return s.GM(ctx, ".respawn") } diff --git a/cmd/turtle-agent/dungeon_leader_pulltest.go b/cmd/turtle-agent/dungeon_leader_pulltest.go index aafd8a57..80935b77 100644 --- a/cmd/turtle-agent/dungeon_leader_pulltest.go +++ b/cmd/turtle-agent/dungeon_leader_pulltest.go @@ -2,8 +2,6 @@ package main import ( "context" - "fmt" - "time" "github.com/pjerra/turtle-agent/internal/brain" "github.com/pjerra/turtle-agent/internal/dungeon" @@ -16,28 +14,7 @@ import ( // Respawn sends the character's own ".respawn" with nothing selected (dungeon.Respawner), as the commander's does: the selection is cleared // first and the server is waited for to show it cleared, else the line would act on that one unit alone. func (g *sessionGame) Respawn(ctx context.Context) error { - s := g.cur() - if err := s.ClearTarget(ctx); err != nil { - return err - } - timer := time.NewTimer(gmSelectConfirmWait) - defer timer.Stop() - tick := time.NewTicker(25 * time.Millisecond) - defer tick.Stop() - for { - if sel, ok := s.ServerSelection(); ok && sel == 0 { - break - } - select { - case <-ctx.Done(): - return ctx.Err() - case <-timer.C: - sel, _ := s.ServerSelection() - return fmt.Errorf("the server did not show the selection cleared within %s (it shows %v): not sending .respawn", gmSelectConfirmWait, sel) - case <-tick.C: - } - } - return s.GM(ctx, ".respawn") + return respawnUnselected(ctx, g.cur()) } // CombatMark is the position of the event log now (dungeon.CombatMeter). diff --git a/cmd/turtle-agent/select_confirm_test.go b/cmd/turtle-agent/select_confirm_test.go index 61e458b6..0b2de144 100644 --- a/cmd/turtle-agent/select_confirm_test.go +++ b/cmd/turtle-agent/select_confirm_test.go @@ -3,6 +3,7 @@ package main import ( "context" "errors" + "fmt" "strings" "sync" "testing" @@ -205,22 +206,157 @@ func TestGMLineIsNotSentWhenTheConfirmedSelectionChanged(t *testing.T) { for _, c := range chats(r, 300*time.Millisecond) { t.Fatalf("a line was sent: %q", c) } - // Selected and shown: the line goes out right after the test's own selection; GMOnConfirmed adds no selection packet of its own. + // Selected again, but the server has not shown it: it still shows the lead from before the stranger was selected, and that stale echo + // (sent again) does not confirm the new select of the lead (issue #296). if _, err := r.s.Target(context.Background(), lead, ""); err != nil { t.Fatal(err) } + r.srv.Send(opcode.SMSGUpdateObject, selfValues(lead)) + time.Sleep(100 * time.Millisecond) + if r.s.SelectionConfirmed(lead) { + t.Fatal("a stale echo of the lead confirmed the new select") + } + if err := r.s.GMOnConfirmed(context.Background(), lead, ".additem 9240"); !errors.Is(err, session.ErrSelectionChanged) { + t.Fatalf("stale echo: %v", err) + } + // The server shows the stranger, then the lead: now the line goes out, and GMOnConfirmed adds no selection packet of its own. + r.srv.Send(opcode.SMSGUpdateObject, selfValues(stranger)) + r.srv.Send(opcode.SMSGUpdateObject, selfValues(lead)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) if err := r.s.GMOnConfirmed(context.Background(), lead, ".additem 9240"); err != nil { t.Fatal(err) } - var ops []uint16 + if ops := selOps(r, 300*time.Millisecond); len(ops) != 2 || ops[0] != "sel 0x23a" || ops[1] != "chat" { + t.Fatalf("ops %v", ops) + } +} + +// selOps are the selections (as "sel 0x..") and chat lines (as "chat") the server received, in order, until nothing came for d. +func selOps(r *agentRig, d time.Duration) []string { + var out []string for { - p, err := r.srv.Next(300 * time.Millisecond) + p, err := r.srv.Next(d) if err != nil { - break + return out + } + switch p.Op { + case opcode.CMSGSetSelection: + out = append(out, fmt.Sprintf("sel %#x", uint64(packet.NewReader(p.Body).GUID()))) + case opcode.CMSGMessagechat: + out = append(out, "chat") + } + } +} + +// The selection-log rule (session.SelectionConfirmed): an echo confirms a select only when it cannot answer an older one. +func TestSelectionEchoMustAnswerTheSelectItConfirms(t *testing.T) { + const lead, stranger = packet.GUID(0x23A), packet.GUID(0x4D2) + + // A fresh echo: the server showed nothing selected, the lead is selected, the echo shows it. + r := newRig(t) + r.srv.Send(opcode.SMSGUpdateObject, selfBlock(0)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(0) }) + if _, err := r.s.Target(context.Background(), lead, ""); err != nil { + t.Fatal(err) + } + if r.s.SelectionConfirmed(lead) { + t.Fatal("confirmed before any echo") + } + r.srv.Send(opcode.SMSGUpdateObject, selfValues(lead)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + + // A stale echo: lead, then stranger, then lead again are sent; an echo of the lead may answer the first, with the stranger still + // pending, so it confirms nothing; the stranger's echo and then the lead's do. + r = newRig(t) + r.srv.Send(opcode.SMSGUpdateObject, selfBlock(0)) + for _, g := range []packet.GUID{lead, stranger, lead} { + if _, err := r.s.Target(context.Background(), g, ""); err != nil { + t.Fatal(err) } - ops = append(ops, p.Op) } - if n := len(ops); n != 2 || ops[0] != opcode.CMSGSetSelection || ops[1] != opcode.CMSGMessagechat { + r.srv.Send(opcode.SMSGUpdateObject, selfValues(lead)) + time.Sleep(100 * time.Millisecond) + if r.s.SelectionConfirmed(lead) || r.s.SelectionConfirmed(stranger) { + t.Fatal("a stale echo confirmed a selection") + } + if !r.s.SelectionAmbiguous(lead) { + t.Fatal("lead before stranger, unresolved: a new select of the lead is ambiguous") + } + r.srv.Send(opcode.SMSGUpdateObject, selfValues(stranger)) + time.Sleep(100 * time.Millisecond) + if r.s.SelectionConfirmed(lead) || r.s.SelectionConfirmed(stranger) { + t.Fatal("the stranger's echo, with the lead sent after it, confirmed a selection") + } + r.srv.Send(opcode.SMSGUpdateObject, selfValues(lead)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + if r.s.SelectionAmbiguous(lead) { + t.Fatal("ambiguous after the log was resolved") + } +} + +// The core sends no update when a selection does not change the target field: selecting the unit already confirmed is confirmed at once, +// with no echo, and the line goes out without a wait. +func TestGMLineOnTheUnchangedSelectionNeedsNoEcho(t *testing.T) { + const lead = packet.GUID(0x23A) + r := newRig(t) + r.srv.Send(opcode.SMSGUpdateObject, selfBlock(lead)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + g := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} + start := time.Now() + for i := 0; i < 3; i++ { + if err := g.selectThenGM(context.Background(), lead, ".levelup 2"); err != nil { + t.Fatal(err) + } + } + if d := time.Since(start); d > time.Second { + t.Fatalf("three lines on the unchanged selection took %s", d) + } + if ops := selOps(r, 300*time.Millisecond); len(ops) != 6 || ops[1] != "chat" || ops[3] != "chat" || ops[5] != "chat" { + t.Fatalf("ops %v", ops) + } +} + +// A core that applies several selections in one update echoes only the last value, and only when it changed. With the lead confirmed and a +// stranger's select pending, an echo of a new select of the lead could not be told from the old one: selectConfirmed selects none first, +// waits for that echo, then selects the lead, and the line runs on the lead. +func TestGMLineSelectsNeutralFirstWhenAnEchoWouldBeAmbiguous(t *testing.T) { + const lead, stranger = packet.GUID(0x23A), packet.GUID(0x4D2) + r := newRig(t) + r.srv.Send(opcode.SMSGUpdateObject, selfBlock(lead)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + if _, err := r.s.Target(context.Background(), stranger, ""); err != nil { // pending: the server has not shown it + t.Fatal(err) + } + if ops := selOps(r, 100*time.Millisecond); len(ops) != 1 || ops[0] != "sel 0x4d2" { + t.Fatalf("ops %v", ops) + } + g := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} + done := make(chan error, 1) + go func() { done <- g.selectThenGM(context.Background(), lead, ".levelup 2") }() + inForce, shown := lead, lead + var ops []string + for len(ops) < 3 { + p, err := r.srv.Next(2 * time.Second) + if err != nil { + t.Fatalf("ops %v: %v", ops, err) + } + switch p.Op { + case opcode.CMSGSetSelection: + // The stranger's select and this one are applied in one update; the echo shows the last value, if it changed. + inForce = packet.NewReader(p.Body).GUID() + ops = append(ops, fmt.Sprintf("sel %#x", uint64(inForce))) + if inForce != shown { + shown = inForce + r.srv.Send(opcode.SMSGUpdateObject, selfValues(inForce)) + } + case opcode.CMSGMessagechat: + ops = append(ops, fmt.Sprintf("chat on %#x", uint64(inForce))) + } + } + if err := <-done; err != nil { + t.Fatal(err) + } + if ops[0] != "sel 0x0" || ops[1] != "sel 0x23a" || ops[2] != "chat on 0x23a" { t.Fatalf("ops %v", ops) } } diff --git a/internal/session/actions.go b/internal/session/actions.go index 87822002..91badb21 100644 --- a/internal/session/actions.go +++ b/internal/session/actions.go @@ -251,6 +251,7 @@ func (s *Session) Target(ctx context.Context, g packet.GUID, name string) (objec } s.mu.Lock() s.target = g + s.selSentLocked(g) s.mu.Unlock() return nil, nil }) @@ -726,6 +727,7 @@ func (s *Session) onNewWorld(body []byte) error { s.mu.Lock() s.mapID, s.pos = nw.Map, [4]float32{nw.X, nw.Y, nw.Z, nw.O} s.target = 0 + s.selUnknownLocked() self := s.self.GUID s.mu.Unlock() s.stopWalk(errNewMap, false) // after the position, so a walk_path woken by it reads the new one diff --git a/internal/session/chat.go b/internal/session/chat.go index 92cf7fd3..4b8cb341 100644 --- a/internal/session/chat.go +++ b/internal/session/chat.go @@ -168,6 +168,7 @@ func (s *Session) GMOn(ctx context.Context, g packet.GUID, text string) error { } s.mu.Lock() s.target = g + s.selSentLocked(g) s.mu.Unlock() return nil, s.conn.WritePacket(opcode.CMSGMessagechat, body) }) @@ -179,8 +180,8 @@ func (s *Session) GMOn(ctx context.Context, g packet.GUID, text string) error { var ErrSelectionChanged = errors.New("gm: the selection is not the confirmed unit any more") // GMOnConfirmed sends the GM command text, and nothing else, as ONE operation of the session's owner goroutine, if the unit g is the selection the -// caller confirmed: the session believes g is selected (no request selected another unit since) AND the server shows g as the player's selection -// (ServerSelection). It sends no CMSG_SET_SELECTION of its own: the core handles a selection on the map thread and a GM chat line on the world +// caller confirmed: the session believes g is selected (no request selected another unit since) AND the server is known to have g in force +// (SelectionConfirmed: an echo that cannot answer an older select, selection.go). It sends no CMSG_SET_SELECTION of its own: the core handles a selection on the map thread and a GM chat line on the world // thread, so a selection sent right before the line would not be in force when the line runs, and the line would act on whatever the server showed // before. When the selection is no longer the confirmed one it returns ErrSelectionChanged with nothing sent, and the caller selects and confirms // again (see the commander's selectThenGM). @@ -205,9 +206,9 @@ func (s *Session) GMOnConfirmed(ctx context.Context, g packet.GUID, text string) return nil, err } s.mu.Lock() - believed := s.target + ok := s.target == g && s.selConfirmedLocked(g) s.mu.Unlock() - if sel, ok := s.ServerSelection(); believed != g || !ok || sel != g { + if !ok { return nil, ErrSelectionChanged } return nil, s.conn.WritePacket(opcode.CMSGMessagechat, body) @@ -229,6 +230,7 @@ func (s *Session) ClearTarget(ctx context.Context) error { } s.mu.Lock() s.target = 0 + s.selSentLocked(0) s.mu.Unlock() return nil, nil }) diff --git a/internal/session/selection.go b/internal/session/selection.go new file mode 100644 index 00000000..a972de29 --- /dev/null +++ b/internal/session/selection.go @@ -0,0 +1,155 @@ +package session + +import ( + "github.com/pjerra/turtle-agent/internal/objects" + "github.com/pjerra/turtle-agent/internal/packet" +) + +// The selection log ties the server's echo of the selection (UNIT_FIELD_TARGET of the player's own object) to the CMSG_SET_SELECTION packets +// the session sent, so that a stale echo cannot confirm a selection that is still on its way (issue #296: an echo showing the lead from an +// earlier select was taken for the new one while a stranger's select, sent in between, was what the server had in force when the GM line ran). +// +// What the session relies on: the core applies the selections of one session in the order they were sent, and an update that carries the +// target field shows the selection in force after some prefix of them (several applied in one map update give one echo, and a selection that +// does not change the field gives none). The core sets a player's selection only from CMSG_SET_SELECTION, except when it replaces the whole +// object (a create block), which is handled as an echo of a value it may have set itself. +// +// selLog[0] is the selection known to be in force at a lower bound of the server's progress through the sent selections (selBaseKnown says +// whether it is known at all: not before the first create block of the player, nor after a far teleport); selLog[1:] are the selections sent +// since, any of which may still be pending. An echo of v moves the lower bound to the FIRST entry that is v (the oldest send it can answer), +// and the selection g is confirmed only when every entry from there on is g: whichever of them the server has reached, g is in force, and +// stays in force until another selection is sent. A selection sent again without change is confirmed at once, with no echo needed. + +// selSentLocked records a CMSG_SET_SELECTION of g written to the server. The caller holds s.mu. +func (s *Session) selSentLocked(g packet.GUID) { + if len(s.selLog) == 0 { + s.selLog = []packet.GUID{0} + s.selBaseKnown = false + } + if n := len(s.selLog); s.selLog[n-1] == g && (n > 1 || s.selBaseKnown) { + return // the same unit again: an echo of it answers the older send as well, so the log need not grow + } + s.selLog = append(s.selLog, g) +} + +// selObservedLocked records that the server shows v as the player's selection. The caller holds s.mu. +func (s *Session) selObservedLocked(v packet.GUID) { + if len(s.selLog) == 0 { + s.selLog, s.selBaseKnown = []packet.GUID{v}, true + return + } + if !s.selBaseKnown { + // The value at the lower bound was unknown, so the echo may be it: nothing sent since is known to be applied. + s.selLog[0], s.selBaseKnown = v, true + return + } + for i, x := range s.selLog { + if x == v { + s.selLog = s.selLog[i:] + return + } + } + // A value the session never sent: the server set it (a create block). It is in force now; what was sent since the lower bound may + // still be pending. + s.selLog[0] = v +} + +// selUnknownLocked forgets the selection in force (a far teleport replaces the player's object); the selections sent stay pending. The +// caller holds s.mu. +func (s *Session) selUnknownLocked() { + if len(s.selLog) == 0 { + s.selLog = []packet.GUID{0} + } + s.selBaseKnown = false +} + +func (s *Session) selConfirmedLocked(g packet.GUID) bool { + if !s.selBaseKnown || len(s.selLog) == 0 { + return false + } + for _, x := range s.selLog { + if x != g { + return false + } + } + return true +} + +// SelectionConfirmed says whether the server is known to have the unit g in force as the player's selection: it showed g in an echo that +// can only answer a selection of g sent no earlier than the last selection of anything else. A stale echo (one showing g from before another +// unit was selected) does not confirm it. +func (s *Session) SelectionConfirmed(g packet.GUID) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.selConfirmedLocked(g) +} + +// SelectionAmbiguous says whether an echo of a new selection of g could not be told from an older one: g was sent, and something else after +// it, and the server has not shown how far it got. Selecting another unit first (SelectionNeutral) and waiting for it to be confirmed +// removes the doubt; without that the core, which may apply several selections in one update and echo only the last, could leave g +// unconfirmable. +func (s *Session) SelectionAmbiguous(g packet.GUID) bool { + s.mu.Lock() + defer s.mu.Unlock() + seen := false + for i, x := range s.selLog { + if i == 0 && !s.selBaseKnown { + continue + } + if x == g { + seen = true + } else if seen { + return true + } + } + return false +} + +// SelectionNeutral is a selection that no entry of the log holds, so its echo answers that send alone: no selection (0) or else the player +// itself. ok is false when the log holds both. +func (s *Session) SelectionNeutral() (g packet.GUID, ok bool) { + s.mu.Lock() + defer s.mu.Unlock() + for _, c := range []packet.GUID{0, s.self.GUID} { + in := false + for _, x := range s.selLog { + if x == c { + in = true + break + } + } + if !in { + return c, true + } + } + return 0, false +} + +// observeSelection records the selection the server shows when one of blocks replaced the player's object or carried its target field. +// Run after the store applied them; the store's value is the last of them. +func (s *Session) observeSelection(blocks []objects.Block, self packet.GUID) { + touched := false + for _, b := range blocks { + if b.GUID != self || self == 0 { + continue + } + switch b.Type { + case objects.BlockCreate, objects.BlockCreate2: + touched = true + case objects.BlockValues: + _, lo := b.Values[objects.UnitTarget] + _, hi := b.Values[objects.UnitTarget+1] + touched = touched || lo || hi + } + } + if !touched { + return + } + u, ok := s.store.Get(self, 0, 0, 0) + if !ok { + return + } + s.mu.Lock() + s.selObservedLocked(u.Target) + s.mu.Unlock() +} diff --git a/internal/session/session.go b/internal/session/session.go index 171b411d..2b34559c 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -203,23 +203,25 @@ type Session struct { clockFn func() time.Time // nil: the wall clock; a replay sets the time of the record it handles afterTake func() // tests only: runs in takeAll between the take and the close - mu sync.Mutex // guards these fields for readers outside the loop - state Status - self Character - mapID uint32 - pos [4]float32 - walkDist float64 // yards along its path the last walk got to when it stopped, arrived or was aborted; read by walk_path - latency uint32 - unknown map[uint16]int - parseErrs int - lastErr string - party PartyInfo - offer *resurrectOffer // the open resurrection offer, death.go - reclaimAt time.Time // the corpse may be reclaimed from here, death.go - marks [game.RaidIconCount]packet.GUID // the group's raid icons - target packet.GUID - falling bool // s.fall != nil, for Status - zUnknown bool + mu sync.Mutex // guards these fields for readers outside the loop + state Status + self Character + mapID uint32 + pos [4]float32 + walkDist float64 // yards along its path the last walk got to when it stopped, arrived or was aborted; read by walk_path + latency uint32 + unknown map[uint16]int + parseErrs int + lastErr string + party PartyInfo + offer *resurrectOffer // the open resurrection offer, death.go + reclaimAt time.Time // the corpse may be reclaimed from here, death.go + marks [game.RaidIconCount]packet.GUID // the group's raid icons + target packet.GUID + selLog []packet.GUID // the selections sent and the one known in force, selection.go + selBaseKnown bool + falling bool // s.fall != nil, for Status + zUnknown bool // owner-loop only finished bool diff --git a/internal/session/world.go b/internal/session/world.go index 1456ea38..6825263f 100644 --- a/internal/session/world.go +++ b/internal/session/world.go @@ -134,6 +134,7 @@ func (s *Session) applyBlocks(blocks []objects.Block) { s.seedRunSpeed(blocks) changes := s.store.Apply(blocks) self := s.selfGUID() + s.observeSelection(blocks, self) var selfAuras []uint32 // positive auras the player gained in this update: a mount's is among them for _, c := range changes { if c.GUID == self && c.Kind == objects.ChangeAura && c.New != 0 && c.Slot < objects.PositiveAuraSlots { From 701ef2debf0a86302f21b6409f9d384c335d0a69 Mon Sep 17 00:00:00 2001 From: pjerra <71784785+pjerra@users.noreply.github.com> Date: Fri, 23 Oct 2026 08:44:38 +0200 Subject: [PATCH 2/4] Selection echo: a combat rewrite of the target field is not a selection (#296 review) UNIT_FIELD_TARGET is also written by Unit::Attack, Unit::AttackStop and a stun, without touching the selection GM commands read. An echo of a value the session never sent now leaves the selection in force unknown, and a repeated select is confirmed with no echo only when the base came from an echo of the session's own send; otherwise a neutral selection is made first (one the log, the dropped sends and the field do not hold). The log is capped at 32 sends; an echo of a dropped unit confirms nothing. The confirmed select lives in the session (SelectConfirmed); GMOn (.aura world buffs) goes through it and GMOnConfirmed, and .respawn is sent with GMOnConfirmed on a confirmed clear. --- README.md | 2 +- cmd/turtle-agent/commander_methods.go | 71 ++------- cmd/turtle-agent/select_confirm_test.go | 126 ++++++++++++++- internal/dungeon/buffs.go | 2 +- internal/dungeon/driver.go | 4 +- internal/session/chat.go | 53 +++---- internal/session/chat_test.go | 114 ++++++++----- internal/session/selection.go | 203 ++++++++++++++++++------ internal/session/selection_test.go | 201 +++++++++++++++++++++++ internal/session/session.go | 5 +- 10 files changed, 597 insertions(+), 184 deletions(-) create mode 100644 internal/session/selection_test.go diff --git a/README.md b/README.md index 59c549ae..3a780560 100644 --- a/README.md +++ b/README.md @@ -2011,7 +2011,7 @@ part before the bind in one go, then the kit check and the enter phase run as be (a bot that carries the item already gets none and nothing is booked), selects the bot by GUID (the agent's own group or view for a hired bot, the roster for a named one; the selection is confirmed by the server first and the line goes out as one session operation, like `.instance unbind all`), sends **`.additem `** and reads `.bot inv` again until the item shows (at most 8 s, else the run fails saying the line may have run on another player or the bags are full). `lead` is the lead bot, `carrier` the run's `carrier` option, else the bot the object steps use (the first bot that is not the lead, tank or healer). It **refuses without a GM account of level 3** (`.additem` is a developer's command; the account level is read with `.account`, and the run stops before it sends or binds anything). The line is a setup shortcut: `additem 9240 on (test setup: Mallet of Zul'Farrak)` in `setup_shortcuts`, - so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited (an echo that could answer an older select of the unit, with another unit's select sent after it, does not count; when a new echo could not be told from such an old one, the selection is first cleared and that confirmed; reselecting the unit already confirmed needs no echo), and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below. + so an honest-run check flags it. `give_items` is refused in group and observe mode. Before anything is hired, logged in or sent, the run checks that the agent's game can do what its options need (`give_items`: `.additem` and `.bot inv`; the prepare block; a hired party; an optional leg's bag read) and fails naming the missing ability, so a run that cannot give items does not pay for a hire first. The `.additem` line (like `.instance unbind all`) acts on the selected player and is sent as the chat packet alone: the unit is selected, the server's echo of the selection is awaited (an echo that could answer an older select of the unit, with another unit's select sent after it, does not count, nor the target field rewritten by combat or a stun, which leaves the selection unknown; when a new echo could not be told from an old one, or would not come because the field already shows the unit, a neutral selection (none, or the agent itself) is made and confirmed first; reselecting the unit already confirmed needs no echo; `.aura` world buffs and `.respawn` go the same way), and the line goes out only while that confirmed selection still stands (the core handles a selection and a GM chat line on different threads, so a selection sent right before the line would not be in force); if another request changed the selection meanwhile, nothing is sent and the unit is selected and confirmed again (three tries). It is how a test gets the Mallet for the optional gahzrilla leg below. Every GM line the step sends is a **setup shortcut**: it is in `setup_shortcuts` (for example `.tele name Lcwarra uldaman (prepare fresh)`, `.instance unbind all on Lcwarra (prepare fresh)`, `gm visible off`) and is a `gm_shortcut` event of kind `setup` in the trace, so `trace-score`'s diff --git a/cmd/turtle-agent/commander_methods.go b/cmd/turtle-agent/commander_methods.go index 5f314986..82d4fcd5 100644 --- a/cmd/turtle-agent/commander_methods.go +++ b/cmd/turtle-agent/commander_methods.go @@ -376,60 +376,15 @@ func (g remoteGame) GMVisible(ctx context.Context, on bool) error { // gmSelectConfirmWait is how long the server is given to show a selection before a line that acts on it is given up on. var gmSelectConfirmWait = 3 * time.Second -// selectConfirmed selects the unit and waits until the SERVER is known to have it in force as the agent's selection -// (session.SelectionConfirmed: the target field of the agent's own object, which the core sends back after it handled CMSG_SET_SELECTION, -// in an echo that cannot be an older one). The core queues that packet (map thread) apart from a GM's chat line (world thread), so a line -// sent right after can run on the selection from before: none (the sender itself) or a stale one, which can be another player. Run 6b: -// `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra. Issue #296: an echo still showing the unit from -// an earlier select, with another unit's select pending behind it, is not a confirmation. When an echo of the new select could not be told -// from such an older one (the unit was selected before another unit, and the server has not shown how far it got), a neutral selection -// (none, or the agent itself) is made and confirmed first. An error, with nothing sent, when the server does not show the selection in time. +// selectConfirmed selects the unit (0: none) and waits until the SERVER is known to have it in force as the agent's selection +// (session.SelectConfirmed: an echo of the target field of the agent's own object that answers this select and not an older one, nor a +// combat or stun rewrite of the field). The core queues that packet (map thread) apart from a GM's chat line (world thread), so a line sent +// right after can run on the selection from before: none (the sender itself) or a stale one, which can be another player. Run 6b: +// `.instance groupunbind all` answered "Glacienne is not in a group" for a selected Lcwarra; issue #296: a stale echo of the lead, with a +// stranger's select pending behind it, let `.additem` run on the stranger. An error, with nothing sent, when the server does not show the +// selection in time. func (g remoteGame) selectConfirmed(ctx context.Context, guid packet.GUID) error { - s := g.c.cur() - timer := time.NewTimer(gmSelectConfirmWait) - defer timer.Stop() - if s.SelectionAmbiguous(guid) { - if n, ok := s.SelectionNeutral(); ok { - if err := selectUnit(ctx, s, n); err != nil { - return err - } - if err := awaitSelection(ctx, s, n, timer.C); err != nil { - return fmt.Errorf("%w (the neutral selection made before %v)", err, guid) - } - } - } - if err := selectUnit(ctx, s, guid); err != nil { - return err - } - return awaitSelection(ctx, s, guid, timer.C) -} - -// selectUnit sends the selection of the unit, or of none for 0. -func selectUnit(ctx context.Context, s *session.Session, guid packet.GUID) error { - if guid == 0 { - return s.ClearTarget(ctx) - } - _, err := s.Target(ctx, guid, "") - return err -} - -// awaitSelection waits until the session says the server has the unit in force as the agent's selection, or until expired fires. -func awaitSelection(ctx context.Context, s *session.Session, guid packet.GUID, expired <-chan time.Time) error { - tick := time.NewTicker(10 * time.Millisecond) - defer tick.Stop() - for { - if s.SelectionConfirmed(guid) { - return nil - } - select { - case <-ctx.Done(): - return ctx.Err() - case <-expired: - sel, _ := s.ServerSelection() - return fmt.Errorf("the server did not show %v as the agent's selection within %s (it shows %v): not sending a line that acts on the selection", guid, gmSelectConfirmWait, sel) - case <-tick.C: - } - } + return g.c.cur().SelectConfirmed(ctx, guid, gmSelectConfirmWait) } // gmSelectAttempts is how many times selectThenGM selects and confirms again when the selection changed after the confirmation. @@ -597,14 +552,14 @@ func (g remoteGame) Respawn(ctx context.Context) error { return respawnUnselected(ctx, g.c.cur()) } -// respawnUnselected clears the selection, waits until the server is known to have it cleared (selectConfirmed with no unit, so a stale echo -// of an earlier clear does not count), and sends ".respawn". +// respawnUnselected clears the selection, waits until the server is known to have it cleared (a stale echo of an earlier clear, or the +// field cleared by the end of a fight, does not count), and sends ".respawn" only while that stands (selectThenGM with no unit). func respawnUnselected(ctx context.Context, s *session.Session) error { g := remoteGame{c: &commanderService{cur: func() *session.Session { return s }}} - if err := g.selectConfirmed(ctx, 0); err != nil { - return fmt.Errorf("the selection was not cleared, not sending .respawn: %w", err) + if err := g.selectThenGM(ctx, 0, ".respawn"); err != nil { + return fmt.Errorf("respawn with nothing selected: %w", err) } - return s.GM(ctx, ".respawn") + return nil } // WalkSelf is walk_path for the agent's character. diff --git a/cmd/turtle-agent/select_confirm_test.go b/cmd/turtle-agent/select_confirm_test.go index 0b2de144..1ce99ce0 100644 --- a/cmd/turtle-agent/select_confirm_test.go +++ b/cmd/turtle-agent/select_confirm_test.go @@ -59,6 +59,7 @@ func TestGMLineWaitsForTheServerToShowTheSelection(t *testing.T) { // The server shows a stranger as selected and never changes it. r := newRig(t) r.srv.Send(opcode.SMSGUpdateObject, selfBlock(stranger)) + waitUntil(t, func() bool { g, ok := r.s.ServerSelection(); return ok && g == stranger }) // the field is known: no neutral selection g := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} err := g.selectThenGM(context.Background(), lead, ".instance groupunbind all") if err == nil || !strings.Contains(err.Error(), "did not show") { @@ -71,6 +72,7 @@ func TestGMLineWaitsForTheServerToShowTheSelection(t *testing.T) { // The selection lands late (after 150 ms): the line goes out after it. r = newRig(t) r.srv.Send(opcode.SMSGUpdateObject, selfBlock(stranger)) + waitUntil(t, func() bool { g, ok := r.s.ServerSelection(); return ok && g == stranger }) // the field is known: no neutral selection g = remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} go func() { time.Sleep(150 * time.Millisecond) @@ -192,8 +194,7 @@ func TestGMLineRunsOnTheConfirmedSelectionWhenSelectionAndChatAreOnSeparateThrea func TestGMLineIsNotSentWhenTheConfirmedSelectionChanged(t *testing.T) { const lead, stranger = packet.GUID(0x23A), packet.GUID(0x4D2) r := newRig(t) - r.srv.Send(opcode.SMSGUpdateObject, selfBlock(lead)) // the server shows the lead... - waitUntil(t, func() bool { g, ok := r.s.ServerSelection(); return ok && g == lead }) + ownSelection(t, r, lead) // the server shows the lead... if _, err := r.s.Target(context.Background(), stranger, ""); err != nil { // ...but another request selected the stranger since t.Fatal(err) } @@ -255,7 +256,10 @@ func TestSelectionEchoMustAnswerTheSelectItConfirms(t *testing.T) { // A fresh echo: the server showed nothing selected, the lead is selected, the echo shows it. r := newRig(t) r.srv.Send(opcode.SMSGUpdateObject, selfBlock(0)) - waitUntil(t, func() bool { return r.s.SelectionConfirmed(0) }) + waitUntil(t, func() bool { _, ok := r.s.ServerSelection(); return ok }) + if r.s.SelectionConfirmed(0) { + t.Fatal("a create block confirmed a selection the session never sent") + } if _, err := r.s.Target(context.Background(), lead, ""); err != nil { t.Fatal(err) } @@ -299,8 +303,7 @@ func TestSelectionEchoMustAnswerTheSelectItConfirms(t *testing.T) { func TestGMLineOnTheUnchangedSelectionNeedsNoEcho(t *testing.T) { const lead = packet.GUID(0x23A) r := newRig(t) - r.srv.Send(opcode.SMSGUpdateObject, selfBlock(lead)) - waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + ownSelection(t, r, lead) g := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} start := time.Now() for i := 0; i < 3; i++ { @@ -322,8 +325,7 @@ func TestGMLineOnTheUnchangedSelectionNeedsNoEcho(t *testing.T) { func TestGMLineSelectsNeutralFirstWhenAnEchoWouldBeAmbiguous(t *testing.T) { const lead, stranger = packet.GUID(0x23A), packet.GUID(0x4D2) r := newRig(t) - r.srv.Send(opcode.SMSGUpdateObject, selfBlock(lead)) - waitUntil(t, func() bool { return r.s.SelectionConfirmed(lead) }) + ownSelection(t, r, lead) if _, err := r.s.Target(context.Background(), stranger, ""); err != nil { // pending: the server has not shown it t.Fatal(err) } @@ -361,6 +363,116 @@ func TestGMLineSelectsNeutralFirstWhenAnEchoWouldBeAmbiguous(t *testing.T) { } } +// ownSelection makes g the confirmed selection the way a run does: the create block (which says nothing about the selection), the +// session's own select of g and the server's echo of it. The select packet is read off the wire. +func ownSelection(t *testing.T, r *agentRig, g packet.GUID) { + t.Helper() + r.srv.Send(opcode.SMSGUpdateObject, selfBlock(0x777)) + waitUntil(t, func() bool { _, ok := r.s.ServerSelection(); return ok }) + var err error + if g == 0 { + err = r.s.ClearTarget(context.Background()) + } else { + _, err = r.s.Target(context.Background(), g, "") + } + if err != nil { + t.Fatal(err) + } + r.srv.Send(opcode.SMSGUpdateObject, selfValues(g)) + waitUntil(t, func() bool { return r.s.SelectionConfirmed(g) }) + if ops := selOps(r, 100*time.Millisecond); len(ops) != 1 { + t.Fatalf("ops %v", ops) + } +} + +// coreLike plays the core for the selection: it applies each CMSG_SET_SELECTION as it reads it and echoes the target field when it +// changed; rewrite changes the FIELD alone (combat, a stun), not the selection; a chat line is recorded with the selection in force. +type coreLike struct { + r *agentRig + sel packet.GUID // what GM commands act on + field packet.GUID // UNIT_FIELD_TARGET as the client last saw it + ops []string +} + +// run reads until want ops (selections and chat lines) were seen. +func (c *coreLike) run(t *testing.T, want int) { + t.Helper() + for len(c.ops) < want { + p, err := c.r.srv.Next(2 * time.Second) + if err != nil { + t.Fatalf("ops %v: %v", c.ops, err) + } + switch p.Op { + case opcode.CMSGSetSelection: + c.sel = packet.NewReader(p.Body).GUID() + c.ops = append(c.ops, fmt.Sprintf("sel %#x", uint64(c.sel))) + if c.sel != c.field { + c.field = c.sel + c.r.srv.Send(opcode.SMSGUpdateObject, selfValues(c.field)) + } + case opcode.CMSGMessagechat: + c.ops = append(c.ops, fmt.Sprintf("chat on %#x", uint64(c.sel))) + } + } +} + +func (c *coreLike) rewrite(t *testing.T, field packet.GUID) { + t.Helper() + c.field = field + c.r.srv.Send(opcode.SMSGUpdateObject, selfValues(field)) + waitUntil(t, func() bool { g, _ := c.r.s.ServerSelection(); return g == field }) +} + +// Unit::AttackStop (the mob the agent fought and had selected died) writes 0 into the target field and leaves the selection on the mob. The +// old dedupe took that 0 for a cleared selection, so `.respawn` went out on the selected mob before the clear landed. Now the clear goes +// through a neutral selection (the agent itself: the field already shows 0, so a clear would get no echo), and the line runs with nothing +// selected. +func TestRespawnAfterAttackStopWaitsForTheRealClear(t *testing.T) { + const mob = packet.GUID(0xF130000000000077) + r := newRig(t) + ownSelection(t, r, mob) + c := &coreLike{r: r, sel: mob, field: mob} + c.rewrite(t, 0) // AttackStop + if r.s.SelectionConfirmed(0) { + t.Fatal("the combat rewrite confirmed a cleared selection") + } + done := make(chan error, 1) + go func() { + done <- remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}}.Respawn(context.Background()) + }() + c.run(t, 3) + if err := <-done; err != nil { + t.Fatal(err) + } + if want := []string{"sel 0x2a", "sel 0x0", "chat on 0x0"}; strings.Join(c.ops, ",") != strings.Join(want, ",") { + t.Fatalf("ops %v, want %v", c.ops, want) + } +} + +// Unit::Attack writes the victim into the field without selecting it: with nothing selected, an attack on the lead shows the lead. A line +// for the lead must not go out on that; it selects the lead through a neutral selection (the field already shows the lead, so a select of +// it would get no echo) and runs on the lead. +func TestGMLineAfterAnAttackOnThePlayerSelectsItForReal(t *testing.T) { + const lead = packet.GUID(0x23A) + r := newRig(t) + ownSelection(t, r, 0) + c := &coreLike{r: r, sel: 0, field: 0} + c.rewrite(t, lead) // Attack + if r.s.SelectionConfirmed(lead) { + t.Fatal("the attack confirmed the victim as the selection") + } + g := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} + done := make(chan error, 1) + go func() { done <- g.selectThenGM(context.Background(), lead, ".levelup 2") }() + c.run(t, 3) + if err := <-done; err != nil { + t.Fatal(err) + } + if want := []string{"sel 0x2a", "sel 0x23a", "chat on 0x23a"}; strings.Join(c.ops, ",") != strings.Join(want, ",") { + t.Fatalf("ops %v, want %v", c.ops, want) + } +} + func waitUntil(t *testing.T, ok func() bool) { t.Helper() for i := 0; i < 200; i++ { diff --git a/internal/dungeon/buffs.go b/internal/dungeon/buffs.go index 0cfbce13..809fded6 100644 --- a/internal/dungeon/buffs.go +++ b/internal/dungeon/buffs.go @@ -173,7 +173,7 @@ func (d *Driver) leaderHas(list []Buff) bool { } // sendBuffs has the leader put each of list on the member with ".aura ", every application flagged in the report -// as a "buff" shortcut. Each command goes with the selection of the member as ONE session operation (Game.GMOn): the +// as a "buff" shortcut. Each command goes only once the server has confirmed the selection of the member (Game.GMOn): the // core runs .aura on whatever is selected when it arrives, and the target RPC or the addon host could change the // selection between a separate Target and GM call (the world buffs would land on a boss). // diff --git a/internal/dungeon/driver.go b/internal/dungeon/driver.go index 0f5f4949..cc850c7d 100644 --- a/internal/dungeon/driver.go +++ b/internal/dungeon/driver.go @@ -35,8 +35,8 @@ type Game interface { Chat(ctx context.Context, channel, text string) error // GM runs a GM command (".revive Name"). GM(ctx context.Context, text string) error - // GMOn selects the unit and runs a GM command that acts on the selected unit (".aura ") as one operation, - // so that nothing else can change the selection between the two. + // GMOn selects the unit, waits until the server is known to have that selection in force, and runs a GM command that + // acts on the selected unit (".aura ") only while it stands; an error, with nothing sent, when it is not confirmed. GMOn(ctx context.Context, guid packet.GUID, text string) error Teleport(ctx context.Context, to Point, mapID uint32) error // ProbeAccount sends ".account" (SEC_PLAYER, Chat.cpp:77) and reads the account's security level from the server's reply diff --git a/internal/session/chat.go b/internal/session/chat.go index 4b8cb341..e94a9e1c 100644 --- a/internal/session/chat.go +++ b/internal/session/chat.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "strings" + "time" "github.com/pjerra/turtle-agent/internal/events" "github.com/pjerra/turtle-agent/internal/game" @@ -139,40 +140,35 @@ func gmLine(text string) (string, error) { return text, nil } -// GMOn selects the unit g and sends the GM command text as ONE operation of the session's owner goroutine: no other -// request (a target call, the addon host's click) can change the selection between the two packets. Use it for a -// command that acts on the selected unit (".aura ", ".revive"), which the core runs on whatever is selected -// when it arrives (Chat.cpp:2329-2340). +// GMOnSelectWait is how long GMOn gives the server to show the selection before the line is given up on. +var GMOnSelectWait = 3 * time.Second + +// gmOnAttempts is how many times GMOn selects and confirms again when the selection changed after the confirmation. +const gmOnAttempts = 3 + +// GMOn selects the unit g, waits until the server is known to have it in force (SelectConfirmed), and sends the GM command text with +// GMOnConfirmed: only while that confirmed selection still stands, with no selection packet of its own. Use it for a command that acts on +// the selected unit (".aura ", ".revive"), which the core runs on whatever is selected when it arrives (Chat.cpp:2329-2340); the core +// handles a selection on the map thread and a chat line on the world thread, so a line sent right after its selection can run on the one +// before. If another request changed the selection after the confirmation, the unit is selected and confirmed again (gmOnAttempts times). +// An error, with nothing sent, when the server does not show the selection in time. func (s *Session) GMOn(ctx context.Context, g packet.GUID, text string) error { if g == 0 { return errors.New("gm: no unit to select") } - text, err := gmLine(text) - if err != nil { + if _, err := gmLine(text); err != nil { return err } - say, err := game.ChatTypeByName("say") - if err != nil { - return err - } - body, err := game.BuildMessageChat(say, "", text) - if err != nil { - return err - } - _, err = s.do(ctx, func() (any, error) { - if err := s.inWorld(); err != nil { - return nil, err + var err error + for try := 0; try < gmOnAttempts; try++ { + if err = s.SelectConfirmed(ctx, g, GMOnSelectWait); err != nil { + return err } - if err := s.conn.WritePacket(opcode.CMSGSetSelection, game.BuildSetSelection(g)); err != nil { - return nil, err + if err = s.GMOnConfirmed(ctx, g, text); !errors.Is(err, ErrSelectionChanged) { + return err } - s.mu.Lock() - s.target = g - s.selSentLocked(g) - s.mu.Unlock() - return nil, s.conn.WritePacket(opcode.CMSGMessagechat, body) - }) - return err + } + return fmt.Errorf("%w: it changed %d times between the server's confirmation and the line, which was not sent", err, gmOnAttempts) } // ErrSelectionChanged is GMOnConfirmed's refusal: the selection is not the unit the caller confirmed (another request changed it, or the @@ -184,11 +180,8 @@ var ErrSelectionChanged = errors.New("gm: the selection is not the confirmed uni // (SelectionConfirmed: an echo that cannot answer an older select, selection.go). It sends no CMSG_SET_SELECTION of its own: the core handles a selection on the map thread and a GM chat line on the world // thread, so a selection sent right before the line would not be in force when the line runs, and the line would act on whatever the server showed // before. When the selection is no longer the confirmed one it returns ErrSelectionChanged with nothing sent, and the caller selects and confirms -// again (see the commander's selectThenGM). +// again (see GMOn and the commander's selectThenGM). g may be 0: a line that must run with nothing selected (".respawn"). func (s *Session) GMOnConfirmed(ctx context.Context, g packet.GUID, text string) error { - if g == 0 { - return errors.New("gm: no unit to select") - } text, err := gmLine(text) if err != nil { return err diff --git a/internal/session/chat_test.go b/internal/session/chat_test.go index 963d0d74..8f24ea19 100644 --- a/internal/session/chat_test.go +++ b/internal/session/chat_test.go @@ -5,15 +5,16 @@ import ( "context" "errors" "fmt" + "strings" "sync" "testing" "time" "github.com/pjerra/turtle-agent/internal/events" "github.com/pjerra/turtle-agent/internal/game" + "github.com/pjerra/turtle-agent/internal/objects" "github.com/pjerra/turtle-agent/internal/opcode" "github.com/pjerra/turtle-agent/internal/packet" - "github.com/pjerra/turtle-agent/internal/worldconn" ) // chatBody transcribes ChatHandler::BuildChatPacket's default and say branches (Chat.cpp:2251-2314). @@ -210,27 +211,50 @@ func TestUnansweredNameDoesNotStallAgain(t *testing.T) { h.logout() } -// GMOn selects the unit and sends the command in one step of the session: however many target calls run at the same -// time, the packet before each command is the selection of the unit it was meant for. -func TestGMOnSelectsAndSendsAtomically(t *testing.T) { +// targetEcho is the values update of the player's own object with the target field the core writes after a selection. +func targetEcho(g packet.GUID) []byte { + return valuesUpdate(me.GUID, map[uint16]uint32{objects.UnitTarget: uint32(g), objects.UnitTarget + 1: uint32(g >> 32)}) +} + +// GMOn selects the unit, waits for the server to show that selection, and sends the command only while it stands: however many target +// calls run at the same time, every command runs on the unit it was meant for. The server here applies each selection as it reads it and +// echoes it when the field changes (as the core does); a command runs on the selection in force when it is read. +func TestGMOnRunsOnlyOnTheConfirmedSelection(t *testing.T) { ctx := context.Background() h := start(t, Options{}) h.enterWorld() - const mine, other, n = packet.GUID(0x31), packet.GUID(0x99), 40 + h.send(opcode.SMSGUpdateObject, unitCreate(me.GUID, objects.TypePlayer, me.X, me.Y, me.Z, map[uint16]uint32{objects.UnitHealth: 800, objects.UnitMaxHealth: 800})) + const mine, other, n = packet.GUID(0x31), packet.GUID(0x99), 20 - var got []worldconn.Packet - collected := make(chan struct{}) + var mu sync.Mutex + var ranOn []packet.GUID + var texts []string + stopSrv := make(chan struct{}) + srvDone := make(chan struct{}) go func() { - defer close(collected) - chats := 0 - for chats < n { - p, err := h.srv.Next(3 * time.Second) - if err != nil { + defer close(srvDone) + inForce := packet.GUID(0) + for { + select { + case <-stopSrv: return + default: + } + p, err := h.srv.Next(50 * time.Millisecond) + if err != nil { + continue } - got = append(got, p) - if p.Op == opcode.CMSGMessagechat { - chats++ + switch p.Op { + case opcode.CMSGSetSelection: + if g := packet.NewReader(p.Body).GUID(); g != inForce { + inForce = g + h.send(opcode.SMSGUpdateObject, targetEcho(g)) + } + case opcode.CMSGMessagechat: + mu.Lock() + ranOn = append(ranOn, inForce) + texts = append(texts, string(p.Body)) + mu.Unlock() } } }() @@ -243,38 +267,31 @@ func TestGMOnSelectsAndSendsAtomically(t *testing.T) { select { case <-stop: return - default: + case <-time.After(15 * time.Millisecond): _, _ = h.s.Target(ctx, other, "") } } }() + sent := 0 for i := 0; i < n; i++ { - if err := h.s.GMOn(ctx, mine, fmt.Sprintf("aura %d", 1000+i)); err != nil { - t.Fatal(err) + if err := h.s.GMOn(ctx, mine, fmt.Sprintf("aura %d", 1000+i)); err == nil { + sent++ } } close(stop) wg.Wait() - <-collected - - cmds := 0 - for i, p := range got { - if p.Op != opcode.CMSGMessagechat { - continue - } - cmds++ - if i == 0 || got[i-1].Op != opcode.CMSGSetSelection || packet.NewReader(got[i-1].Body).GUID() != mine { - t.Fatalf("command %d (%q) was not right after the selection of its unit", cmds, p.Body) + time.Sleep(200 * time.Millisecond) + close(stopSrv) + <-srvDone + mu.Lock() + defer mu.Unlock() + if sent == 0 || len(ranOn) != sent { + t.Fatalf("%d sent, %d ran", sent, len(ranOn)) + } + for i, g := range ranOn { + if g != mine { + t.Errorf("command %d (%q) ran on %v", i, texts[i], g) } - if !bytes.Contains(p.Body, []byte(fmt.Sprintf(".aura %d", 1000+cmds-1))) { - t.Fatalf("command %d: % X", cmds, p.Body) - } - } - if cmds != n { - t.Fatalf("%d commands seen", cmds) - } - if h.s.Status().Target == 0 { - t.Fatal("no target") } for _, bad := range []string{"", "..gm"} { if err := h.s.GMOn(ctx, mine, bad); err == nil { @@ -286,6 +303,29 @@ func TestGMOnSelectsAndSendsAtomically(t *testing.T) { } } +// GMOn sends nothing when the server never shows the selection. +func TestGMOnSendsNothingWithoutTheEcho(t *testing.T) { + old := GMOnSelectWait + GMOnSelectWait = 200 * time.Millisecond + defer func() { GMOnSelectWait = old }() + ctx := context.Background() + h := start(t, Options{}) + h.enterWorld() + h.send(opcode.SMSGUpdateObject, unitCreate(me.GUID, objects.TypePlayer, me.X, me.Y, me.Z, map[uint16]uint32{objects.UnitHealth: 800, objects.UnitMaxHealth: 800})) + if err := h.s.GMOn(ctx, 0x31, ".aura 1"); err == nil || !strings.Contains(err.Error(), "did not show") { + t.Fatalf("%v", err) + } + for { + p, err := h.srv.Next(200 * time.Millisecond) + if err != nil { + break + } + if p.Op == opcode.CMSGMessagechat { + t.Fatalf("a line went out: %q", p.Body) + } + } +} + // ClearTarget sends the selection of nothing, as a client does when the target is dropped, and the session no longer believes in one. func TestClearTargetSendsAZeroSelection(t *testing.T) { ctx := context.Background() diff --git a/internal/session/selection.go b/internal/session/selection.go index a972de29..848fcae5 100644 --- a/internal/session/selection.go +++ b/internal/session/selection.go @@ -1,7 +1,13 @@ package session import ( + "context" + "fmt" + "time" + + "github.com/pjerra/turtle-agent/internal/game" "github.com/pjerra/turtle-agent/internal/objects" + "github.com/pjerra/turtle-agent/internal/opcode" "github.com/pjerra/turtle-agent/internal/packet" ) @@ -9,49 +15,81 @@ import ( // the session sent, so that a stale echo cannot confirm a selection that is still on its way (issue #296: an echo showing the lead from an // earlier select was taken for the new one while a stranger's select, sent in between, was what the server had in force when the GM line ran). // -// What the session relies on: the core applies the selections of one session in the order they were sent, and an update that carries the -// target field shows the selection in force after some prefix of them (several applied in one map update give one echo, and a selection that -// does not change the field gives none). The core sets a player's selection only from CMSG_SET_SELECTION, except when it replaces the whole -// object (a create block), which is handled as an echo of a value it may have set itself. +// What a GM command acts on is the player's selection GUID (Player::GetSelectionGuid, Chat.cpp:2334), which only CMSG_SET_SELECTION sets. The +// field the session can see is UNIT_FIELD_TARGET: the core writes it on that packet too, but ALSO without touching the selection GUID when +// the player starts or stops attacking (Unit::Attack Unit.cpp:5189 writes the victim, Unit::AttackStop :5257 writes 0, also through +// RemoveAllAttackers on the victim's death) and around a stun (SpellAuras.cpp:3359 writes 0, :3412 the victim again). So the field is evidence +// of the selection only where it answers a select the session sent: +// - the core applies the selections of one session in the order they were sent, and an update that carries the field shows it after some +// prefix of them (several applied in one map update give one echo, and a select that does not change the field gives none); +// - an echo of a value the session never sent (a combat or stun rewrite, a create block) leaves the selection in force UNKNOWN; the +// sends after the lower bound stay pending. +// What is left: a combat rewrite to exactly the unit of a pending select reads as that select's echo. // -// selLog[0] is the selection known to be in force at a lower bound of the server's progress through the sent selections (selBaseKnown says -// whether it is known at all: not before the first create block of the player, nor after a far teleport); selLog[1:] are the selections sent -// since, any of which may still be pending. An echo of v moves the lower bound to the FIRST entry that is v (the oldest send it can answer), -// and the selection g is confirmed only when every entry from there on is g: whichever of them the server has reached, g is in force, and -// stays in force until another selection is sent. A selection sent again without change is confirmed at once, with no echo needed. +// selLog[0] is the selection known to be in force at a lower bound of the server's progress through the sent selections, known only when an +// echo answered one of the session's own sends (selBaseKnown); selLog[1:] are the selections sent since, any of which may still be pending. +// An echo of v moves the lower bound to the FIRST pending entry that is v (the oldest send it can answer), and the selection g is confirmed +// only when every entry from there on is g: whichever of them the server has reached, g is in force, and stays in force until another +// selection is sent. Selecting g again while g is confirmed is confirmed at once, with no echo needed (the core sends none). Where an echo +// of a new select could not be told from an older one, or would not come at all because the field already shows the unit, SelectConfirmed +// first makes and confirms a neutral selection. + +// selCap bounds the log: the selections sent with no echo between them. Past it the oldest are dropped, the selection in force becomes +// unknown, and an echo of a dropped unit confirms nothing until an echo of a later send was seen. +const selCap = 32 // selSentLocked records a CMSG_SET_SELECTION of g written to the server. The caller holds s.mu. func (s *Session) selSentLocked(g packet.GUID) { if len(s.selLog) == 0 { - s.selLog = []packet.GUID{0} - s.selBaseKnown = false + s.selLog, s.selBaseKnown = []packet.GUID{0}, false } if n := len(s.selLog); s.selLog[n-1] == g && (n > 1 || s.selBaseKnown) { return // the same unit again: an echo of it answers the older send as well, so the log need not grow } s.selLog = append(s.selLog, g) + if len(s.selLog) > selCap { + drop := len(s.selLog) - (selCap - 1) // entries leaving the log; the new base is unknown + if s.selDropped == nil { + s.selDropped = map[packet.GUID]bool{} + } + for i, x := range s.selLog[:drop] { + if i > 0 || s.selBaseKnown { + s.selDropped[x] = true + } + } + kept := append([]packet.GUID{0}, s.selLog[drop:]...) + s.selLog, s.selBaseKnown = kept, false + } } -// selObservedLocked records that the server shows v as the player's selection. The caller holds s.mu. +// selObservedLocked records that the server shows v in the player's target field. The caller holds s.mu. func (s *Session) selObservedLocked(v packet.GUID) { + s.selShown, s.selShownOK = v, true if len(s.selLog) == 0 { - s.selLog, s.selBaseKnown = []packet.GUID{v}, true + s.selLog, s.selBaseKnown = []packet.GUID{0}, false // nothing sent: the field says nothing about the selection return } - if !s.selBaseKnown { - // The value at the lower bound was unknown, so the echo may be it: nothing sent since is known to be applied. - s.selLog[0], s.selBaseKnown = v, true - return + start := 1 + if s.selBaseKnown { + start = 0 } - for i, x := range s.selLog { - if x == v { - s.selLog = s.selLog[i:] + for i := start; i < len(s.selLog); i++ { + if s.selLog[i] != v { + continue + } + if s.selDropped[v] { + // It may answer a dropped send, with everything after it pending. + s.selBaseKnown = false return } + s.selLog, s.selBaseKnown = s.selLog[i:], true + if i > 0 { + s.selDropped = nil // a kept send was applied, so every dropped one was too + } + return } - // A value the session never sent: the server set it (a create block). It is in force now; what was sent since the lower bound may - // still be pending. - s.selLog[0] = v + // A value no pending send holds: the core rewrote the field itself (combat, a stun, a create block). The selection in force is unknown. + s.selBaseKnown = false } // selUnknownLocked forgets the selection in force (a far teleport replaces the player's object); the selections sent stay pending. The @@ -60,7 +98,7 @@ func (s *Session) selUnknownLocked() { if len(s.selLog) == 0 { s.selLog = []packet.GUID{0} } - s.selBaseKnown = false + s.selBaseKnown, s.selShownOK = false, false } func (s *Session) selConfirmedLocked(g packet.GUID) bool { @@ -75,22 +113,16 @@ func (s *Session) selConfirmedLocked(g packet.GUID) bool { return true } -// SelectionConfirmed says whether the server is known to have the unit g in force as the player's selection: it showed g in an echo that -// can only answer a selection of g sent no earlier than the last selection of anything else. A stale echo (one showing g from before another -// unit was selected) does not confirm it. -func (s *Session) SelectionConfirmed(g packet.GUID) bool { - s.mu.Lock() - defer s.mu.Unlock() - return s.selConfirmedLocked(g) -} - -// SelectionAmbiguous says whether an echo of a new selection of g could not be told from an older one: g was sent, and something else after -// it, and the server has not shown how far it got. Selecting another unit first (SelectionNeutral) and waiting for it to be confirmed -// removes the doubt; without that the core, which may apply several selections in one update and echo only the last, could leave g -// unconfirmable. -func (s *Session) SelectionAmbiguous(g packet.GUID) bool { - s.mu.Lock() - defer s.mu.Unlock() +// selNeedsNeutralLocked says whether a new select of g could stay unconfirmed: its echo could not be told from an older one (g was sent, +// then something else, and the server has not shown how far it got), or no echo would come (the selection in force is unknown and the field +// already shows g, or the field is not known). +func (s *Session) selNeedsNeutralLocked(g packet.GUID) bool { + if s.selConfirmedLocked(g) { + return false + } + if !s.selBaseKnown && (!s.selShownOK || s.selShown == g) { + return true + } seen := false for i, x := range s.selLog { if i == 0 && !s.selBaseKnown { @@ -105,12 +137,13 @@ func (s *Session) SelectionAmbiguous(g packet.GUID) bool { return false } -// SelectionNeutral is a selection that no entry of the log holds, so its echo answers that send alone: no selection (0) or else the player -// itself. ok is false when the log holds both. -func (s *Session) SelectionNeutral() (g packet.GUID, ok bool) { - s.mu.Lock() - defer s.mu.Unlock() +// selNeutralLocked is a selection whose echo answers its own send alone: one no entry of the log (or a dropped one) holds and the field does +// not show, so the field must change. No selection (0) or else the player itself; ok is false when neither will do. +func (s *Session) selNeutralLocked() (packet.GUID, bool) { for _, c := range []packet.GUID{0, s.self.GUID} { + if s.selDropped[c] || (s.selShownOK && s.selShown == c) { + continue + } in := false for _, x := range s.selLog { if x == c { @@ -125,8 +158,84 @@ func (s *Session) SelectionNeutral() (g packet.GUID, ok bool) { return 0, false } -// observeSelection records the selection the server shows when one of blocks replaced the player's object or carried its target field. -// Run after the store applied them; the store's value is the last of them. +// SelectionConfirmed says whether the server is known to have the unit g (0: none) in force as the player's selection: an echo that can only +// answer one of the session's own selects of g, sent no earlier than its last select of anything else, showed it. A stale echo, or the +// field rewritten by combat or a stun, does not confirm it. +func (s *Session) SelectionConfirmed(g packet.GUID) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.selConfirmedLocked(g) +} + +// SelectionAmbiguous says whether SelectConfirmed would make a neutral selection before selecting g. +func (s *Session) SelectionAmbiguous(g packet.GUID) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.selNeedsNeutralLocked(g) +} + +// sendSelection writes CMSG_SET_SELECTION of g (0: none) from the owner goroutine and records it. +func (s *Session) sendSelection(ctx context.Context, g packet.GUID) error { + _, err := s.do(ctx, func() (any, error) { + if err := s.inWorld(); err != nil { + return nil, err + } + if err := s.conn.WritePacket(opcode.CMSGSetSelection, game.BuildSetSelection(g)); err != nil { + return nil, err + } + s.mu.Lock() + s.target = g + s.selSentLocked(g) + s.mu.Unlock() + return nil, nil + }) + return err +} + +// SelectConfirmed selects the unit g (0: none) and waits, up to wait in all, until the server is known to have it in force +// (SelectionConfirmed). When a new select of g could stay unconfirmed (SelectionAmbiguous), a neutral selection (none, or the player itself) +// is made and confirmed first. An error when the confirmation does not come in time; a line that acts on the selection must not be sent then. +func (s *Session) SelectConfirmed(ctx context.Context, g packet.GUID, wait time.Duration) error { + timer := time.NewTimer(wait) + defer timer.Stop() + s.mu.Lock() + need := s.selNeedsNeutralLocked(g) + n, ok := s.selNeutralLocked() + s.mu.Unlock() + if need && ok { + if err := s.sendSelection(ctx, n); err != nil { + return err + } + if err := s.awaitSelection(ctx, n, wait, timer.C); err != nil { + return fmt.Errorf("%w (the neutral selection made before %v)", err, g) + } + } + if err := s.sendSelection(ctx, g); err != nil { + return err + } + return s.awaitSelection(ctx, g, wait, timer.C) +} + +func (s *Session) awaitSelection(ctx context.Context, g packet.GUID, wait time.Duration, expired <-chan time.Time) error { + tick := time.NewTicker(10 * time.Millisecond) + defer tick.Stop() + for { + if s.SelectionConfirmed(g) { + return nil + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-expired: + sel, _ := s.ServerSelection() + return fmt.Errorf("the server did not show %v as the agent's selection within %s (it shows %v): not sending a line that acts on the selection", g, wait, sel) + case <-tick.C: + } + } +} + +// observeSelection records the target field the server shows when one of blocks replaced the player's object or carried the field. Run +// after the store applied them; the store's value is the last of them. func (s *Session) observeSelection(blocks []objects.Block, self packet.GUID) { touched := false for _, b := range blocks { diff --git a/internal/session/selection_test.go b/internal/session/selection_test.go new file mode 100644 index 00000000..020c18f2 --- /dev/null +++ b/internal/session/selection_test.go @@ -0,0 +1,201 @@ +package session + +import ( + "testing" + + "github.com/pjerra/turtle-agent/internal/packet" +) + +// selRig drives the selection log of a bare session: sends and echoes, no connection. +type selRig struct{ s *Session } + +func newSelRig() selRig { + s := &Session{} + s.self.GUID = 0x2A + return selRig{s} +} + +func (r selRig) sent(gs ...packet.GUID) { + r.s.mu.Lock() + defer r.s.mu.Unlock() + for _, g := range gs { + r.s.selSentLocked(g) + } +} + +func (r selRig) echo(v packet.GUID) { + r.s.mu.Lock() + defer r.s.mu.Unlock() + r.s.selObservedLocked(v) +} + +func (r selRig) neutral() (packet.GUID, bool) { + r.s.mu.Lock() + defer r.s.mu.Unlock() + return r.s.selNeutralLocked() +} + +const ( + selLead = packet.GUID(0x23A) + selStranger = packet.GUID(0x4D2) + selMob = packet.GUID(0xF130000000000077) +) + +// own makes g the confirmed selection by an echo of the session's own send. +func (r selRig) own(t *testing.T, g packet.GUID) { + t.Helper() + r.echo(0x999) // the create block: says nothing about the selection + r.sent(g) + r.echo(g) + if !r.s.SelectionConfirmed(g) { + t.Fatalf("%v not confirmed by the echo of its own send", g) + } +} + +func TestSelectionLogFreshEchoConfirms(t *testing.T) { + r := newSelRig() + r.echo(0) // the create block + if r.s.SelectionConfirmed(0) { + t.Fatal("a create block confirmed a selection") + } + r.sent(selLead) + if r.s.SelectionConfirmed(selLead) { + t.Fatal("confirmed before an echo") + } + r.echo(selLead) + if !r.s.SelectionConfirmed(selLead) { + t.Fatal("the fresh echo did not confirm") + } +} + +func TestSelectionLogStaleEchoDoesNotConfirm(t *testing.T) { + r := newSelRig() + r.own(t, selLead) + r.sent(selStranger, selLead) + r.echo(selLead) // may answer the first select, with the stranger's pending + if r.s.SelectionConfirmed(selLead) { + t.Fatal("a stale echo confirmed") + } + if !r.s.SelectionAmbiguous(selLead) { + t.Fatal("not ambiguous") + } + r.echo(selStranger) + if r.s.SelectionConfirmed(selLead) || r.s.SelectionConfirmed(selStranger) { + t.Fatal("the stranger's echo, with the lead sent after it, confirmed") + } + r.echo(selLead) + if !r.s.SelectionConfirmed(selLead) { + t.Fatal("the lead's own echo did not confirm") + } +} + +// The core sends no update for a select that does not change the field: reselecting the confirmed unit is confirmed with no echo. +func TestSelectionLogUnchangedSelectionNeedsNoEcho(t *testing.T) { + r := newSelRig() + r.own(t, selLead) + for i := 0; i < 5; i++ { + r.sent(selLead) + } + if !r.s.SelectionConfirmed(selLead) || r.s.SelectionAmbiguous(selLead) { + t.Fatal("the unchanged selection is not confirmed at once") + } + if n := len(r.s.selLog); n != 1 { + t.Fatalf("log grew to %d", n) + } +} + +// Unit::AttackStop writes 0 into the target field (the victim died) without touching the selection the GM commands read: that is not a +// cleared selection, and clearing it now gets no echo (the field does not change), so it must go through a neutral selection. +func TestSelectionLogAttackStopZeroIsNotAClearedSelection(t *testing.T) { + r := newSelRig() + r.own(t, selMob) + r.echo(0) // AttackStop + if r.s.SelectionConfirmed(0) || r.s.SelectionConfirmed(selMob) { + t.Fatal("the combat rewrite confirmed a selection") + } + if !r.s.SelectionAmbiguous(0) { + t.Fatal("clearing would wait for an echo that never comes") + } + if n, ok := r.neutral(); !ok || n != r.s.self.GUID { + t.Fatalf("neutral %v %v: 0 is what the field shows, the player itself must be used", n, ok) + } + r.sent(0) // without the neutral: the clear is recorded, not deduped into a confirmation + if r.s.SelectionConfirmed(0) { + t.Fatal("the clear was confirmed with no echo") + } + // The neutral round trip. + r.sent(r.s.self.GUID) + r.echo(r.s.self.GUID) + if !r.s.SelectionConfirmed(r.s.self.GUID) { + t.Fatal("neutral not confirmed") + } + r.sent(0) + r.echo(0) + if !r.s.SelectionConfirmed(0) { + t.Fatal("the clear after the neutral was not confirmed") + } +} + +// Unit::Attack writes the victim into the field: with nothing selected, an attack on a player shows that player without selecting it. +func TestSelectionLogAttackOnAPlayerIsNotItsSelection(t *testing.T) { + r := newSelRig() + r.own(t, 0) + r.echo(selLead) // Attack + if r.s.SelectionConfirmed(selLead) { + t.Fatal("the attack confirmed the victim as the selection") + } + if !r.s.SelectionAmbiguous(selLead) { + t.Fatal("selecting the victim would wait for an echo that never comes") + } + r.sent(selLead) + if r.s.SelectionConfirmed(selLead) { + t.Fatal("confirmed with no echo") + } + // A stun end restoring the victim, after a stun cleared the field, is no echo of the select either... unless it is: that is the one + // case the log cannot tell apart (a rewrite to exactly the unit of a pending select). + r.echo(0) // stun + if r.s.SelectionConfirmed(0) { + t.Fatal("the stun's 0 confirmed a clear") + } +} + +// The log is capped; an echo of a unit whose send was dropped confirms nothing, and the neutral avoids dropped units. +func TestSelectionLogCap(t *testing.T) { + r := newSelRig() + r.own(t, selLead) + r.sent(0) // will be dropped + for i := 0; i < 2*selCap; i++ { + r.sent(selStranger+packet.GUID(i%7), selLead) + } + if n := len(r.s.selLog); n > selCap { + t.Fatalf("log at %d", n) + } + if r.s.selBaseKnown { + t.Fatal("the base is known after a drop") + } + r.echo(selLead) // may answer a dropped select + if r.s.SelectionConfirmed(selLead) { + t.Fatal("an echo of a dropped unit confirmed") + } + if n, ok := r.neutral(); !ok || n != r.s.self.GUID { + t.Fatalf("neutral %v %v: 0 was dropped", n, ok) + } + // The neutral round trip resets the log. + r.sent(r.s.self.GUID) + r.echo(r.s.self.GUID) + if !r.s.SelectionConfirmed(r.s.self.GUID) || len(r.s.selLog) != 1 || r.s.selDropped != nil { + t.Fatalf("log %v dropped %v", r.s.selLog, r.s.selDropped) + } +} + +// A far teleport forgets the selection in force. +func TestSelectionLogNewWorldForgets(t *testing.T) { + r := newSelRig() + r.own(t, selLead) + r.s.mu.Lock() + r.s.selUnknownLocked() + r.s.mu.Unlock() + if r.s.SelectionConfirmed(selLead) || !r.s.SelectionAmbiguous(selLead) { + t.Fatal("still confirmed after the teleport") + } +} diff --git a/internal/session/session.go b/internal/session/session.go index 2b34559c..b71e3ba7 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -220,7 +220,10 @@ type Session struct { target packet.GUID selLog []packet.GUID // the selections sent and the one known in force, selection.go selBaseKnown bool - falling bool // s.fall != nil, for Status + selShown packet.GUID // the target field the server showed last; selShownOK: one was seen since the object was (re)created + selShownOK bool + selDropped map[packet.GUID]bool // units of sends dropped from the log at selCap + falling bool // s.fall != nil, for Status zUnknown bool // owner-loop only From 4c72af5498abcb4b37991f8a6d3ed57a33ad50be Mon Sep 17 00:00:00 2001 From: pjerra <71784785+pjerra@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:04:08 +0200 Subject: [PATCH 3/4] Selection echo: no confirmation while the agent attacks, fights or is stunned (#296 review 2) The core rewrites the player's target field only through Attack, AttackStop and stun apply/end; a rewrite to exactly the unit of a pending select read as its echo. While the player is hot (its own SMSG_ATTACKSTART until the ATTACKSTOP, UNIT_FLAG_IN_COMBAT or UNIT_FLAG_STUNNED) and for SelectionCombatSettle after, no echo confirms and no GM line on a selection goes out. Cooling down fences off every send so far; SelectConfirmed waits inside its timer, then selects again (through a neutral selection when the unit was fenced off) and needs the echo of that new send. A neutral selection no longer counts the unknown base's placeholder as a unit. --- cmd/turtle-agent/select_confirm_test.go | 72 +++++++++++++- internal/objects/fields.go | 1 + internal/session/actions.go | 7 ++ internal/session/selection.go | 125 ++++++++++++++++++++++-- internal/session/selection_test.go | 48 +++++++++ internal/session/session.go | 5 +- internal/session/world.go | 1 + 7 files changed, 248 insertions(+), 11 deletions(-) diff --git a/cmd/turtle-agent/select_confirm_test.go b/cmd/turtle-agent/select_confirm_test.go index 1ce99ce0..472fe48c 100644 --- a/cmd/turtle-agent/select_confirm_test.go +++ b/cmd/turtle-agent/select_confirm_test.go @@ -468,11 +468,81 @@ func TestGMLineAfterAnAttackOnThePlayerSelectsItForReal(t *testing.T) { if err := <-done; err != nil { t.Fatal(err) } - if want := []string{"sel 0x2a", "sel 0x23a", "chat on 0x23a"}; strings.Join(c.ops, ",") != strings.Join(want, ",") { + if want := []string{"sel 0x0", "sel 0x23a", "chat on 0x23a"}; strings.Join(c.ops, ",") != strings.Join(want, ",") { t.Fatalf("ops %v, want %v", c.ops, want) } } +// selfFlags is a values update of the agent's own UNIT_FIELD_FLAGS (in combat, stunned). +func selfFlags(flags uint32) []byte { + w := objtest.Header(1).U8(uint8(objects.BlockValues)).PackedGUID(0x2A) + objtest.Values(w, 192, map[uint16]uint32{objects.UnitFlags: flags}) + return w.Bytes() +} + +// Codex's case (#296 review): the lead X is confirmed, a select of G is pending, the agent is in combat and the core rewrites the target +// field to G (Unit::Attack). That must not confirm G: the selection the GM line acts on can still be X. Nothing is selected or sent until +// combat ended (and SelectionCombatSettle passed); then G is selected again, through a neutral selection because its old send was fenced +// off, and the line goes out after the echo of that new send. +func TestGMLineWaitsOutCombatWhenTheFieldIsRewrittenToThePendingUnit(t *testing.T) { + old := session.SelectionCombatSettle + session.SelectionCombatSettle = 100 * time.Millisecond + defer func() { session.SelectionCombatSettle = old }() + const x, g = packet.GUID(0x23A), packet.GUID(0x4D2) + r := newRig(t) + ownSelection(t, r, x) + if _, err := r.s.Target(context.Background(), g, ""); err != nil { // pending: the server has not applied it + t.Fatal(err) + } + if ops := selOps(r, 100*time.Millisecond); len(ops) != 1 || ops[0] != "sel 0x4d2" { + t.Fatalf("ops %v", ops) + } + r.srv.Send(opcode.SMSGUpdateObject, selfFlags(objects.UnitFlagInCombat)) + r.srv.Send(opcode.SMSGUpdateObject, selfValues(g)) // the attack's rewrite, exactly the pending unit + waitUntil(t, func() bool { sel, _ := r.s.ServerSelection(); return sel == g }) + gm := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} + done := make(chan error, 1) + go func() { done <- gm.selectThenGM(context.Background(), g, ".levelup 2") }() + if ops := selOps(r, 400*time.Millisecond); len(ops) != 0 { + t.Fatalf("sent while in combat: %v", ops) + } + if r.s.SelectionConfirmed(g) { + t.Fatal("the combat rewrite confirmed the pending unit") + } + cooled := time.Now() + c := &coreLike{r: r, sel: g, field: g} + r.srv.Send(opcode.SMSGUpdateObject, selfFlags(0)) + c.run(t, 3) + if err := <-done; err != nil { + t.Fatal(err) + } + if d := time.Since(cooled); d < 100*time.Millisecond { + t.Fatalf("the line went out %s after combat, inside the settle", d) + } + if want := []string{"sel 0x0", "sel 0x4d2", "chat on 0x4d2"}; strings.Join(c.ops, ",") != strings.Join(want, ",") { + t.Fatalf("ops %v, want %v", c.ops, want) + } +} + +// A fight that outlasts the wait: an error, and nothing selected or sent. +func TestGMLineInCombatPastTheWaitSendsNothing(t *testing.T) { + old := gmSelectConfirmWait + gmSelectConfirmWait = 300 * time.Millisecond + defer func() { gmSelectConfirmWait = old }() + const lead = packet.GUID(0x23A) + r := newRig(t) + ownSelection(t, r, lead) + r.srv.Send(opcode.SMSGUpdateObject, selfFlags(objects.UnitFlagStunned)) + waitUntil(t, func() bool { return !r.s.SelectionConfirmed(lead) }) + gm := remoteGame{c: &commanderService{cur: func() *session.Session { return r.s }}} + if err := gm.selectThenGM(context.Background(), lead, ".levelup 2"); err == nil || !strings.Contains(err.Error(), "in combat or stunned") { + t.Fatalf("%v", err) + } + if ops := selOps(r, 200*time.Millisecond); len(ops) != 0 { + t.Fatalf("ops %v", ops) + } +} + func waitUntil(t *testing.T, ok func() bool) { t.Helper() for i := 0; i < 200; i++ { diff --git a/internal/objects/fields.go b/internal/objects/fields.go index 72cfdea3..5cae29df 100644 --- a/internal/objects/fields.go +++ b/internal/objects/fields.go @@ -64,6 +64,7 @@ const ( UnitFlagSpawning uint32 = 0x00000002 UnitFlagPlayerControlled uint32 = 0x00000008 UnitFlagImmuneToPlayer uint32 = 0x00000100 + UnitFlagStunned uint32 = 0x00040000 UnitFlagInCombat uint32 = 0x00080000 UnitFlagNotSelectable uint32 = 0x02000000 ) diff --git a/internal/session/actions.go b/internal/session/actions.go index 91badb21..622c36eb 100644 --- a/internal/session/actions.go +++ b/internal/session/actions.go @@ -728,6 +728,7 @@ func (s *Session) onNewWorld(body []byte) error { s.mapID, s.pos = nw.Map, [4]float32{nw.X, nw.Y, nw.Z, nw.O} s.target = 0 s.selUnknownLocked() + s.selAttacking = false self := s.self.GUID s.mu.Unlock() s.stopWalk(errNewMap, false) // after the position, so a walk_path woken by it reads the new one @@ -827,6 +828,9 @@ func (s *Session) onAttackStart(body []byte) error { if err != nil { return err } + if a.Attacker == s.selfGUID() { + s.setAttacking(true) // Unit::Attack writes the victim into the target field in the next update + } s.emit(events.Event{Kind: events.KindAttackStart, SourceGUID: a.Attacker, TargetGUID: a.Victim}) return nil } @@ -836,6 +840,9 @@ func (s *Session) onAttackStop(body []byte) error { if err != nil { return err } + if a.Attacker == s.selfGUID() { + s.setAttacking(false) // AttackStop writes 0 into the target field in the next update: SelectionCombatSettle covers it + } s.emit(events.Event{Kind: events.KindAttackStop, SourceGUID: a.Attacker, TargetGUID: a.Victim}) return nil } diff --git a/internal/session/selection.go b/internal/session/selection.go index 848fcae5..49623fb2 100644 --- a/internal/session/selection.go +++ b/internal/session/selection.go @@ -2,6 +2,7 @@ package session import ( "context" + "errors" "fmt" "time" @@ -24,7 +25,15 @@ import ( // prefix of them (several applied in one map update give one echo, and a select that does not change the field gives none); // - an echo of a value the session never sent (a combat or stun rewrite, a create block) leaves the selection in force UNKNOWN; the // sends after the lower bound stay pending. -// What is left: a combat rewrite to exactly the unit of a pending select reads as that select's echo. +// - those rewrites happen only while the player attacks, is in combat or is stunned (the core's other SetTargetGuid calls are for +// creatures, Unit.cpp:9815/9859). A combat rewrite to exactly the unit of a pending select would read as that select's echo, so while +// the player is HOT (attacking since an SMSG_ATTACKSTART of its own until the SMSG_ATTACKSTOP, UNIT_FLAG_IN_COMBAT or UNIT_FLAG_STUNNED +// on its object) and for SelectionCombatSettle after, no echo confirms anything and no GM line on a selection goes out. When the player +// cools down, every send so far is fenced off (treated as dropped: an echo of one of those units confirms nothing until an echo of a +// later send was seen), and SelectConfirmed selects again, through a neutral selection where its unit is among the fenced ones. A run +// that stays hot past the wait gets an error and no line. Left: a rewrite whose sign of combat comes in a LATER update than the rewrite +// itself (none of the core paths above does this for a player: the melee start is announced before the update, the flags of a cast's +// combat and of a stun go in the same update). // // selLog[0] is the selection known to be in force at a lower bound of the server's progress through the sent selections, known only when an // echo answered one of the session's own sends (selBaseKnown); selLog[1:] are the selections sent since, any of which may still be pending. @@ -38,6 +47,57 @@ import ( // unknown, and an echo of a dropped unit confirms nothing until an echo of a later send was seen. const selCap = 32 +// SelectionCombatSettle is how long after the player stopped attacking, left combat or a stun the selection echoes are still not trusted: +// the field rewrite of an AttackStop comes in the update after the SMSG_ATTACKSTOP. +var SelectionCombatSettle = time.Second + +// selHotLocked says whether the field may be rewritten by combat or a stun now. The caller holds s.mu. +func (s *Session) selHotLocked() bool { + return s.selHot || time.Now().Before(s.selCoolAt) +} + +// selSetHotLocked records whether the player attacks, is in combat or is stunned. Cooling down fences off every send so far. The caller +// holds s.mu. +func (s *Session) selSetHotLocked(hot bool) { + if hot == s.selHot { + return + } + s.selHot = hot + if hot { + return + } + s.selCoolAt = time.Now().Add(SelectionCombatSettle) + if s.selDropped == nil { + s.selDropped = map[packet.GUID]bool{} + } + for i, x := range s.selLog { + if i > 0 || s.selBaseKnown { + s.selDropped[x] = true + } + } + s.selLog, s.selBaseKnown = []packet.GUID{0}, false +} + +// updateHot recomputes the hot state from the player's own object and its melee attack. Run before an update's target field is read. +func (s *Session) updateHot(self packet.GUID) { + flags := uint32(0) + if u, ok := s.store.Get(self, 0, 0, 0); ok && self != 0 { + flags = u.Flags + } + s.mu.Lock() + s.selSetHotLocked(s.selAttacking || flags&(objects.UnitFlagInCombat|objects.UnitFlagStunned) != 0) + s.mu.Unlock() +} + +// setAttacking records an SMSG_ATTACKSTART (true) or SMSG_ATTACKSTOP (false) of the player itself. +func (s *Session) setAttacking(on bool) { + s.mu.Lock() + s.selAttacking = on + self := s.self.GUID + s.mu.Unlock() + s.updateHot(self) +} + // selSentLocked records a CMSG_SET_SELECTION of g written to the server. The caller holds s.mu. func (s *Session) selSentLocked(g packet.GUID) { if len(s.selLog) == 0 { @@ -65,6 +125,10 @@ func (s *Session) selSentLocked(g packet.GUID) { // selObservedLocked records that the server shows v in the player's target field. The caller holds s.mu. func (s *Session) selObservedLocked(v packet.GUID) { s.selShown, s.selShownOK = v, true + if s.selHotLocked() { + s.selBaseKnown = false // combat or a stun may have written it + return + } if len(s.selLog) == 0 { s.selLog, s.selBaseKnown = []packet.GUID{0}, false // nothing sent: the field says nothing about the selection return @@ -102,7 +166,7 @@ func (s *Session) selUnknownLocked() { } func (s *Session) selConfirmedLocked(g packet.GUID) bool { - if !s.selBaseKnown || len(s.selLog) == 0 { + if !s.selBaseKnown || len(s.selLog) == 0 || s.selHotLocked() { return false } for _, x := range s.selLog { @@ -120,7 +184,7 @@ func (s *Session) selNeedsNeutralLocked(g packet.GUID) bool { if s.selConfirmedLocked(g) { return false } - if !s.selBaseKnown && (!s.selShownOK || s.selShown == g) { + if !s.selBaseKnown && (!s.selShownOK || s.selShown == g) || s.selDropped[g] { return true } seen := false @@ -145,8 +209,8 @@ func (s *Session) selNeutralLocked() (packet.GUID, bool) { continue } in := false - for _, x := range s.selLog { - if x == c { + for i, x := range s.selLog { + if x == c && (i > 0 || s.selBaseKnown) { // an unknown base is a placeholder, not a unit in = true break } @@ -192,12 +256,28 @@ func (s *Session) sendSelection(ctx context.Context, g packet.GUID) error { return err } +// errSelHot ends a wait for a selection when the player turned hot (selHotLocked): the selection is made again once it cooled down. +var errSelHot = errors.New("the player is attacking, in combat or stunned") + // SelectConfirmed selects the unit g (0: none) and waits, up to wait in all, until the server is known to have it in force // (SelectionConfirmed). When a new select of g could stay unconfirmed (SelectionAmbiguous), a neutral selection (none, or the player itself) -// is made and confirmed first. An error when the confirmation does not come in time; a line that acts on the selection must not be sent then. +// is made and confirmed first. While the player is attacking, in combat or stunned nothing is selected and nothing is confirmed (the core +// rewrites the field then); the selection is made once the player cooled down. An error when the confirmation does not come in time; a +// line that acts on the selection must not be sent then. func (s *Session) SelectConfirmed(ctx context.Context, g packet.GUID, wait time.Duration) error { timer := time.NewTimer(wait) defer timer.Stop() + for { + if err := s.awaitCool(ctx, wait, timer.C); err != nil { + return err + } + if err := s.selectOnce(ctx, g, wait, timer.C); !errors.Is(err, errSelHot) { + return err + } + } +} + +func (s *Session) selectOnce(ctx context.Context, g packet.GUID, wait time.Duration, expired <-chan time.Time) error { s.mu.Lock() need := s.selNeedsNeutralLocked(g) n, ok := s.selNeutralLocked() @@ -206,23 +286,50 @@ func (s *Session) SelectConfirmed(ctx context.Context, g packet.GUID, wait time. if err := s.sendSelection(ctx, n); err != nil { return err } - if err := s.awaitSelection(ctx, n, wait, timer.C); err != nil { + if err := s.awaitSelection(ctx, n, wait, expired); err != nil { return fmt.Errorf("%w (the neutral selection made before %v)", err, g) } } if err := s.sendSelection(ctx, g); err != nil { return err } - return s.awaitSelection(ctx, g, wait, timer.C) + return s.awaitSelection(ctx, g, wait, expired) +} + +// awaitCool waits until the player is not hot. +func (s *Session) awaitCool(ctx context.Context, wait time.Duration, expired <-chan time.Time) error { + tick := time.NewTicker(10 * time.Millisecond) + defer tick.Stop() + for { + s.mu.Lock() + hot := s.selHotLocked() + s.mu.Unlock() + if !hot { + return nil + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-expired: + return fmt.Errorf("the agent was attacking, in combat or stunned for all of %s (the core rewrites its target field then, so no selection can be confirmed): not sending a line that acts on the selection", wait) + case <-tick.C: + } + } } func (s *Session) awaitSelection(ctx context.Context, g packet.GUID, wait time.Duration, expired <-chan time.Time) error { tick := time.NewTicker(10 * time.Millisecond) defer tick.Stop() for { - if s.SelectionConfirmed(g) { + s.mu.Lock() + ok, hot := s.selConfirmedLocked(g), s.selHotLocked() + s.mu.Unlock() + if ok { return nil } + if hot { + return errSelHot + } select { case <-ctx.Done(): return ctx.Err() diff --git a/internal/session/selection_test.go b/internal/session/selection_test.go index 020c18f2..4454b98f 100644 --- a/internal/session/selection_test.go +++ b/internal/session/selection_test.go @@ -2,6 +2,7 @@ package session import ( "testing" + "time" "github.com/pjerra/turtle-agent/internal/packet" ) @@ -199,3 +200,50 @@ func TestSelectionLogNewWorldForgets(t *testing.T) { t.Fatal("still confirmed after the teleport") } } + +func (r selRig) hot(on bool) { + r.s.mu.Lock() + defer r.s.mu.Unlock() + r.s.selSetHotLocked(on) +} + +// Codex's case: X confirmed, G pending, the player in combat, the core rewrites the field to G. That echo confirms nothing, nor does any +// echo while the player is hot or settling; after it cooled down, G needs a neutral selection (its old send was fenced off) and a fresh +// echo of a new send. +func TestSelectionLogCombatRewriteToThePendingUnitConfirmsNothing(t *testing.T) { + old := SelectionCombatSettle + SelectionCombatSettle = 50 * time.Millisecond + defer func() { SelectionCombatSettle = old }() + r := newSelRig() + r.own(t, selLead) // X + r.sent(selStranger) // G, pending + r.hot(true) + r.echo(selStranger) // Unit::Attack on G + if r.s.SelectionConfirmed(selStranger) { + t.Fatal("a combat rewrite confirmed the pending unit") + } + r.hot(false) + r.echo(selStranger) // within the settle: still not trusted + if r.s.SelectionConfirmed(selStranger) { + t.Fatal("confirmed while settling") + } + time.Sleep(60 * time.Millisecond) + r.echo(selStranger) // after it: the old send of G was fenced off + if r.s.SelectionConfirmed(selStranger) { + t.Fatal("an echo of a fenced-off send confirmed") + } + if !r.s.SelectionAmbiguous(selStranger) { + t.Fatal("G was fenced off: a neutral selection is needed") + } + n, ok := r.neutral() + if !ok || n == selStranger || n == selLead { + t.Fatalf("neutral %v %v", n, ok) + } + r.sent(n) + r.echo(n) + r.sent(selStranger) + r.echo(selStranger) + if !r.s.SelectionConfirmed(selStranger) { + t.Fatal("the fresh echo after the neutral did not confirm") + } +} diff --git a/internal/session/session.go b/internal/session/session.go index b71e3ba7..4e7b3313 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -222,7 +222,10 @@ type Session struct { selBaseKnown bool selShown packet.GUID // the target field the server showed last; selShownOK: one was seen since the object was (re)created selShownOK bool - selDropped map[packet.GUID]bool // units of sends dropped from the log at selCap + selDropped map[packet.GUID]bool // units of sends dropped from the log at selCap or fenced off when the player cooled down + selHot bool // attacking, in combat or stunned, selection.go + selCoolAt time.Time // until then the echoes are still not trusted + selAttacking bool // between the player's own SMSG_ATTACKSTART and SMSG_ATTACKSTOP falling bool // s.fall != nil, for Status zUnknown bool diff --git a/internal/session/world.go b/internal/session/world.go index 6825263f..dfa357e4 100644 --- a/internal/session/world.go +++ b/internal/session/world.go @@ -134,6 +134,7 @@ func (s *Session) applyBlocks(blocks []objects.Block) { s.seedRunSpeed(blocks) changes := s.store.Apply(blocks) self := s.selfGUID() + s.updateHot(self) // first: a stun's flag and its field rewrite come in one update s.observeSelection(blocks, self) var selfAuras []uint32 // positive auras the player gained in this update: a mount's is among them for _, c := range changes { From fa88f3924dfe233e2edcb245c5201b64347f6601 Mon Sep 17 00:00:00 2001 From: pjerra <71784785+pjerra@users.noreply.github.com> Date: Fri, 23 Oct 2026 09:22:31 +0200 Subject: [PATCH 4/4] GPS test: allow the neutral selection of nothing before the commander When GPS started before the session had read the create block, the target field was unknown and SelectConfirmed made a neutral selection of nothing first, which the test refused (CI run 39915580906). The test now echoes every selection, allows only one neutral selection of nothing before the commander, and still checks that .gps runs with the commander selected. --- cmd/turtle-agent/commander_gps_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/cmd/turtle-agent/commander_gps_test.go b/cmd/turtle-agent/commander_gps_test.go index 3eaaabaa..42be8bce 100644 --- a/cmd/turtle-agent/commander_gps_test.go +++ b/cmd/turtle-agent/commander_gps_test.go @@ -68,22 +68,27 @@ func TestGPSSelectsTheCommanderBeforeTheLine(t *testing.T) { text, err := g.GPS(context.Background()) done <- reply{text, err} }() + // The server applies each selection and echoes it. A neutral selection of nothing may come first (when GPS starts before the session + // read the create block, the field is unknown: session.SelectConfirmed); anything else, or a line on anything but the commander, fails. var ops []uint16 - for len(ops) < 2 { + inForce := stranger + for len(ops) == 0 || ops[len(ops)-1] != opcode.CMSGMessagechat { p, err := r.srv.Next(2 * time.Second) if err != nil { t.Fatalf("after %v: %v", ops, err) } switch p.Op { case opcode.CMSGSetSelection: - if guid := packet.NewReader(p.Body).GUID(); guid != 0x2A { - t.Fatalf("selected %#x, not the commander (0x2A)", uint64(guid)) + guid := packet.NewReader(p.Body).GUID() + if guid != 0x2A && (guid != 0 || len(ops) != 0) { + t.Fatalf("selected %#x, not the commander (0x2A) or a first neutral selection of nothing", uint64(guid)) } + inForce = guid ops = append(ops, p.Op) - r.srv.Send(opcode.SMSGUpdateObject, selfValues(0x2A)) // the server's echo of the selection + r.srv.Send(opcode.SMSGUpdateObject, selfValues(guid)) // the server's echo of the selection case opcode.CMSGMessagechat: - if len(ops) == 0 { - t.Fatal("the .gps line was sent before the commander was selected") + if inForce != 0x2A { + t.Fatalf("the .gps line was sent with %#x selected, not the commander", uint64(inForce)) } if !strings.Contains(string(p.Body), ".gps") { t.Fatalf("chat line %q", p.Body)