Student/feat/better teleport

[StudentAlleg]

No description provided.

[Copilot]

🟡 Changes recommended

Unresolved protocol compatibility and ripcord synchronization issues remain.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This pull request improves goal-aware ripcord targeting, command-plan synchronization, and route visualization.

Changes:

• Adds ripcord goal data to client/server messages.
• Synchronizes command plans and ripcord state.
• Updates group-command echoing and route rendering.

File summaries

File	Reviewed change and final notes
src/WinTrek/WinTrek.cpp	Sends selected ripcord goal models.
src/WinTrek/trekigc.cpp	Processes self-issued group orders. Moderate (1 vote): group self-echo can apply orders to a non-recipient local ship.
src/WinTrek/console.cpp	Selects group-message echo recipients.
src/WinTrek/cmdview.cpp	Draws routes from active plans. Moderate (1 vote): may draw a route when the actual ripcord destination is unknown.
src/Inc/Messages.h	Extends wire message layouts. Moderate (2 votes): regenerate stale Wopr wrappers. Critical (2 votes): increment MSGVER from 218.
src/Igc/shipIGC.h	Declares ripcord goal APIs.
src/Igc/shipIGC.cpp	Implements ripcord target selection. Moderate (1 vote each): cross-sector targets can override local candidates; route searches ignore cowardly-route preferences.
src/Igc/igc.h	Updates the ship interface.
src/FedSrv/fsship.cpp	Sends command-plan state. Moderate (1 vote): dynamic buoys in other sectors may not be exported, losing the plan.
src/FedSrv/FedSrv.H	Adds the goal-aware request overload.
src/FedSrv/FedSrv.CPP	Handles ripcord requests and replication. Moderate (1 vote): unseen objects may be marked visible without export. Moderate (2 votes): abort notifications may be duplicated. Moderate (1 vote): activation notifications may be duplicated.
src/clintlib/clintlib.h	Serializes client ripcord goals.
src/clintlib/appmsg.cpp	Applies command and ripcord updates. Moderate (1 vote): joining clients may lack the active ripcord target.

Review details

Suppressed comments (8)

src/FedSrv/FedSrv.CPP:13903

• Marking an unseen asteroid/station/warp visible here suppresses the normal export path, but this block never calls ExportObj for those types. A client that only knows the object through this ORDER_CHANGE therefore gets a valid ID but GetModel returns null; because SeenBySide is now true, later spotting/export logic will skip it as well. Export the target to the same recipient before marking it visible, or defer the visibility change until it can be exported.

        IsideIGC*   psideShip = pship->GetSide();
        if (ptarget && psideShip && !ptarget->SeenBySide(psideShip))
        {
            ObjectType  otTarget = ptarget->GetObjectType();
            if ((otTarget == OT_asteroid) || (otTarget == OT_station) || (otTarget == OT_warp))
                ptarget->SetSideVisibility(psideShip, true);

src/FedSrv/FedSrv.CPP:13545

• The side-group send above already reaches the player's own side, and a player currently in pclusterShip is also in GetGroupSectorFlying(pclusterShip). This second send therefore delivers a duplicate RIPCORD_ACTIVATE, causing duplicate ripcord text/effect updates for everyone in that sector; skip this send when both groups are the same recipient.

                if (pfsShip->IsPlayer() && (pmodelOld == NULL))
                    SendRipcordActivated(pship, pmodelNew, pcluster,
                                         GetGroupSectorFlying(pclusterShip));

src/FedSrv/fsship.cpp:874

• A plan can be a dynamic waypoint buoy in a different sector. The buoy export performed when a player enters a sector only sends buoys whose cluster equals the current pcluster, so a client that missed the original ORDER_CHANGE receives this SSU but GetModel(pfmSSU->otPlan, pfmSSU->oidPlan) returns NULL and the new plan is lost. Export referenced dynamic targets before this update (or send enough plan data to reconstruct them).

                        //And the plan, which is the slot the ship is actually flying.
                        ImodelIGC*  pmodelPlan = pshipExist->GetCommandTarget(c_cmdPlan);
                        pfmSSU->otPlan  = pmodelPlan ? pmodelPlan->GetObjectType() : NA;
                        pfmSSU->oidPlan = pmodelPlan ? pmodelPlan->GetObjectID()   : NA;
                        pfmSSU->cidPlan = pshipExist->GetCommandID(c_cmdPlan);

src/Igc/shipIGC.cpp:3923

• PickRipcordGoal returns the first visible selection/order even when neither model is in pcluster, because that model is used for the route test. However, FindRipcordModel immediately treats any returned ship or probe as pmodelRipcord without checking its cluster, so a selected carrier/probe in another sector can make a rip requested into pcluster target that object outside the requested sector and skip all local candidates. Keep a separate in-destination goal for direct target selection, or gate the direct-target branch with pigc->GetCluster(this, pmodelGoal) == pcluster.

	ImodelIGC*      pmodelGoal = PickRipcordGoal(pcluster);

src/Igc/shipIGC.cpp:3783

• These route searches hard-code bCowardly=false, while CommandGeo::BuildRoute passes pship->GetPilotType() < c_ptCarrier for the same route (src/WinTrek/cmdview.cpp:385,466). A miner or fighter can therefore choose a ripcord target using a route through sectors that its actual route avoids, making the selected teleport disagree with the path drawn to the player. Use the ship's cowardly-route preference consistently in both searches.

    PathList*   ppath = FindRouteList(pclusterShip, GetPosition(), GetSide(), pmodelGoal, false);

src/WinTrek/cmdview.cpp:414

• bRipcordOriginKnown only gates CASE 2 below. If the fallback poriginModel == pship is used and ptarget is also in the viewed cluster, CASE 1 still draws a normal ship-to-target route even though the actual rip destination is unknown; this contradicts the new comment that there is no route to draw in that case. Skip route construction for an active ship whose ripcord model is the stand-in, or handle it before the case analysis.

            bool bRipcordOriginKnown = pship->fRipcordActive() &&
                                       (poriginModel != (ImodelIGC*)pship);

src/WinTrek/trekigc.cpp:4868

• BaseClient::SendChat invokes ReceiveChat locally for the single CHAT_GROUP echo even when the local ship is not in the selected group; console.cpp deliberately chooses the first group member as pshipEcho in that case. This condition checks only the recipient type and sender, so the local pilot applies a group order to their own ship despite not being a recipient. Require oidRecipient to equal the local ship for the CHAT_GROUP self-echo case.

            Command cmd = ((ctRecipient == CHAT_INDIVIDUAL) || (ctRecipient == CHAT_GROUP)) &&
                          (pshipSender == trekClient.GetShip())

src/clintlib/appmsg.cpp:1165

• A client entering or re-viewing a sector after the original RIPCORD_ACTIVATE was sent never receives the ripcord target: the snapshot only carries bIsRipcording. This branch therefore installs the ship stand-in even when the teleport is known to the side, so CommandGeo cannot draw the ripcord route for that client. Include the ripcord target in the snapshot or send an activation to the joining connection.

                        if (!pfmSSU->bIsRipcording)
                            ship->SetRipcordModel(NULL);
                        else if (ship->GetRipcordModel() == NULL)
                            ship->SetRipcordModel(ship);    //Just has to be a valid pointer

• Files reviewed: 13/13 changed files
• Comments generated: 3
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🔵 Needs a closer look

Unresolved moderate issues remain in state synchronization, ripcord routing, command echo handling, and ripcord cleanup.

Review details

Suppressed comments (7)

Previously missed (1) — in code that hasn't changed since the last review.

src/WinTrek/trekigc.cpp:4868

• BaseClient::SendChat invokes ReceiveChat locally for every CHAT_GROUP message, including the fallback first-member send used when the sender is not in the group. With this condition, a self-sent group command addressed to another member is treated as c_cmdAccepted, so commanding a group that does not include the player installs the order on the sender too. Restrict the group case to the local echo whose oidRecipient is the player's ship (or otherwise carry the membership/echo information).

src/FedSrv/fsship.cpp:566

• The snapshot is built before the drone cleanup at lines 585-600. When a drone reaches its destination cluster, c_cmdPlan can still be the cluster buoy here even though the server immediately clears it; side clients receive the later ORDER_CHANGE, but allied/sector-only recipients do not and retain a stale plan and buoy consumer. Build the snapshot after that cleanup or suppress the plan for this transition.

                ImodelIGC*  pmodelPlan = m_pShip->GetCommandTarget(c_cmdPlan);
                pfmSSU->otPlan  = pmodelPlan ? pmodelPlan->GetObjectType() : NA;
                pfmSSU->oidPlan = pmodelPlan ? pmodelPlan->GetObjectID()   : NA;
                pfmSSU->cidPlan = m_pShip->GetCommandID(c_cmdPlan);

src/FedSrv/fsship.cpp:566

• This path sends the update straight to GetGroupSectorFlying and does not perform the buoy export that CFSPlayer::SetCluster performs below. If a remote or allied client is already in the destination sector and missed the side's earlier ORDER_CHANGE, an out-of-sector c_cmdPlan buoy cannot be resolved from otPlan/oidPlan, so the client stores a null plan and loses the route. Export the referenced buoy to this recipient before flushing the update, or otherwise guarantee that it is present.

                ImodelIGC*  pmodelPlan = m_pShip->GetCommandTarget(c_cmdPlan);
                pfmSSU->otPlan  = pmodelPlan ? pmodelPlan->GetObjectType() : NA;
                pfmSSU->oidPlan = pmodelPlan ? pmodelPlan->GetObjectID()   : NA;
                pfmSSU->cidPlan = m_pShip->GetCommandID(c_cmdPlan);

src/Igc/shipIGC.cpp:3787

• This recomputes the route from the ship's current position, but the actual route can already have a committed first hop in GetWaypointWarp; CommandGeo::BuildRoute uses that warp before searching the remainder. After a drone has moved far enough for another aleph to become cheaper, this test can mark a sector as on-route when the ship will not fly there, making ripcord target selection diverge from the route shown to players. Incorporate the committed warp before searching the remainder, as BuildRoute does.

    PathList*   ppath = FindRouteList(pclusterShip, GetPosition(), GetSide(), pmodelGoal,
                                      IsCowardlyRoute());

src/Igc/shipIGC.cpp:3754

• PickRipcordGoal only examines c_cmdCurrent and c_cmdAccepted, even though c_cmdPlan is the slot the ship actually flies and is what determines the requested cluster in ResetWaypoint. When a drone/miner has a plan target different from its standing order, ripcord selection ignores that target and falls back to the wrong point (often a dock), so it can choose a teleport away from the route it is following. Include the plan slot with an explicit priority appropriate to player selections.

    //Selection first, then the standing order, so that a specifically selected target still
    //wins when both are in the sector being ripped to.
    ImodelIGC*  pcandidates[2] = { m_commandTargets[c_cmdCurrent], m_commandTargets[c_cmdAccepted] };

src/WinTrek/console.cpp:1497

• If the group contains the local ship, pshipEcho is set to it and this sends CHAT_GROUP. BaseClient::SendChat immediately calls ReceiveChat for that type and also sends the message to the server; the server echoes it back as CHAT_INDIVIDUAL, so the local chat/order is processed twice. Use a no-echo send for the local recipient when connected, or otherwise suppress the local receive while retaining the server echo.

                        (pship == pshipEcho) ? CHAT_GROUP : CHAT_GROUP_NOECHO, 

src/clintlib/appmsg.cpp:1184

• When a landing SINGLE_SHIP_UPDATE arrives before RIPCORD_ABORTED—the server sends the update during SetCluster and the ended message afterward—this clears the model but leaves the visual ripcord countdown active. The later abort handler returns early because the model is already null, so the effect can remain indefinitely. Clear the ripcord timer in this branch as well.

                        if (!pfmSSU->bIsRipcording)
                            ship->SetRipcordModel(NULL);

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Lite