Mod support: a framework for composing gameplay rules with hooks, and CTF and Lithium mods to prove it out - #915
Merged
Merged
Conversation
Sv_InitEntities resolved and dlopen'd game.so synchronously while
mapping, but the new mod's search path was only mounted later, from
Frame(), after the map command had already finished executing. A
`game <mod>; map <name>` issued in the same command batch would load
the previous (often default) game.so instead of the requested mod's.
Call Fs_SetGame synchronously in Sv_InitEntities, right after the
latched game cvar is applied and before Sv_InitGame resolves the
library, so the correct search path is always mounted first.
Also keep the client's game cvar in sync with the server's game
directory in Cl_ParseServerData, which previously remounted the
filesystem and reloaded cgame without updating the cvar itself,
leaving Cvar_GetString("game") stale for consumers like demo headers.
Refs #906
Wires up a second, independently-built game module (ctf.so) as a verbatim copy of src/game/default/, to prove the module-loading fix works for a real second mod: `+set game ctf +map <name>` now mounts the ctf search path and loads ctf/game.so, confirmed by running a dedicated server end-to-end. This is scaffolding only, not the full CTF extraction: the copy still contains all of default's gameplay code unchanged. Stripping non-CTF gameplay out of this module, moving CTF-only fields out of shared structs, and removing CTF code from default are follow-up work. Refs #906
Mirrors the ctf game module with a matching client game module, so a client connecting to a ctf server loads ctf/cgame.so rather than falling back on default's. Include paths, install dirs and the libitem/libpmove references are repointed from default to ctf. Like the game module, this is a verbatim copy for now; stripping it down to CTF-only content is follow-up work. Refs #906
The ctf cgame module carried a verbatim copy of the entire ObjectivelyMVC UI - menus, settings, controls, credits, editor - none of which is mod-specific. Every additional module would have duplicated another ~15k lines, and upstream UI fixes would never reach a mod that had already forked them. Move the UI to src/cgame/shared/ui and have each module compile it via the sources.mk fragment. Sharing sources rather than a prebuilt library is deliberate: cg_state_t embeds cg_team_info_t teams[MAX_TEAMS] plus g_gameplay_t and g_items_t, all of which come from the module's own game/<module>/g_types.h, so a library built against one module's definitions would read cg_state at the wrong offsets in any module that changed the team roster or item set. Compiling per module costs a little build time and removes that failure mode entirely. Each module's convenience library is named for the module, because with subdir-objects automake derives object file names from it and two modules both using the cgame_la- prefix would write colliding objects into the shared source directory. UI assets now install once to @PKGLIBDIR@/ui rather than per module. The engine already mounts lib_dir itself as its first search path, so any module resolves them; verified by hiding the directory, which makes the client fail on ui/fonts/small. Refs #906
The ctf game module carried a byte-identical copy of the func_* brush entities, target_* entities, entity physics and sound emission - roughly 3.7k lines of map and BSP plumbing that a mod has no reason to reshape. Move them to src/game/shared and compile them per module, matching what the cgame UI now does. Per-module compilation is again deliberate rather than incidental: these files include the module's own g_local.h, so g_entity_t, g_client_t and g_level all take that module's definitions. Each module's convenience library is named for the module so automake's subdir-objects naming cannot collide in the shared directory. G_Ripple moves out of g_ballistics.c into a new shared g_effect.c. It is a pure temp-entity emitter with no weapon content, but g_physics.c calls it, so leaving it among the weapon rules would have made shared code depend on a file every mod rewrites. Gameplay stays per module: match flow, scoring, damage, items, weapons, client handling and the structs themselves. The few names shared code calls back into - G_Damage, G_Explode, G_UseTargets and the MOD_CRUSH family - are documented in sources.mk as contract a module must keep. Deliberately not moved yet: g_entity_trigger.c, g_entity_misc.c and g_util.c. Each is coupled to code slated for removal - the dropped flag and tech reset, the grapple, and g_util's team and flag helpers - so sharing them before CTF is stripped out of default would mean doing the work twice. Verified default and ctf both spawn 82 entities on edge, unchanged. Refs #906
The project already uses "common" for shared code, and src/game/shared sat badly next to the engine-wide src/shared, reading as though the two were related. Rename to src/game/common and src/cgame/common, and rename the makefile variables to match: GSHARED_* becomes GCOMMON_*, and the per-module convenience libraries become libgcommon<module>. Directory and variable names only; no build or behavior change.
Techs are a CTF construct - they only spawned when capture play was on, and g_techs defaulted to following g_level.ctf. Deathmatch has no use for them, so they belong to the ctf module alone. Removes the five tech items and their tags, the g_techs cvar and G_CheckTechs, tech spawning and respawn point selection, pickup, drop and dropped-item reset, the haste fire-rate scaling, the resist, strength and vampire multipliers inside G_Damage, the regen tick, STAT_TECH and its HUD icon, the tech sound indices, and the bot's tech goal filter. regen_time went with them, having had no other driver. Techs touched no engine code, so this is confined to the module: the tag enum, the item table, g_types.h and the matching cgame, all rebuilt together. FLAG_FIRST now follows POWERUP_LAST directly. Verified the item table still aligns with the tag enum, which G_InitItems indexes directly: 53 slots for 52 entries plus the ITEM_NONE placeholder. default's module carries no tech symbols or asset strings and 45 item classnames; ctf keeps 50. Both still load and spawn 82 entities on edge. Refs #906
G_CreateTeamSpawnPoints existed to make CTF playable on maps that were never built for it: with no flags present it ran an O(n squared) search for the two furthest-apart deathmatch spawns, repurposed them as flag positions, then partitioned the remaining spawns by distance to each fabricated flag. Its own comment conceded the flags "will be in crap positions". Purpose-built CTF levels supply their own team spawns and flag placement, so the hack has no remaining use. Team play in the default module now simply uses info_player_deathmatch for everyone, which needs no new code: G_SelectRandomSpawnPoint already recurses into g_level.spawn_points when a team's pool is empty. On edge, a deathmatch map, both modules now spawn 80 entities rather than 82 - the two flags this code used to invent.
Player movement is the same physics for every module, and the two copies were still byte-identical. Move bg_pmove.c and its header to src/game/common so a mod inherits movement rather than forking 1400 lines of it. bg_item deliberately stays per module: it is the mod's item roster, and the two copies have already diverged over techs. Movement is inherited, content is owned. The pmove convenience library is named per module, as the other common sources are, because automake's subdir-objects derives object file names from it and a shared name would collide in src/game/common. Compiling per module also keeps the hook branches in pmove honest - they read this module's headers, and stay unreachable unless the module sets pm.s.type to one of the PM_HOOK_* values.
The create-server menu hardcoded free for all, team deathmatch and capture the flag, so every module advertised capture play whether or not it implemented any. It also mapped the modes to cvars by hand, and had a bug doing it: free for all is added with value 0, which fell through to the default case and set g_ctf to 1, so choosing free for all enabled capture the flag. Cases 2 and default were identical. Each cgame module now supplies a small table of the arrangements it actually offers, and the common controller builds its list from that and applies whatever cvars each entry names. The bug goes with the hand mapping: a mode's cvars are stated once, next to the mode. Overriding the whole controller per module was the alternative, but it would fork 191 lines to vary three, leave each fork to miss later fixes to the create-server flow, and repeat for every mod. What varies is the list, not the view, so only the list moves. Cg_TeamModes joins G_Damage and G_Explode as a name a module must provide. default offers two arrangements, ctf three.
Quake, Quake II and Quake III all leave the grapple out of the base game, and default is plain deathmatch, so the hook goes with capture play. It was already gated behind g_hook defaulting to follow g_level.ctf. Removes the hook projectile and its trail, detach, think and fire logic, the eight g_hook cvars, G_CheckHook, the hook style handling and its user_info round trip, MOD_HOOK, TE_HOOK_IMPACT, TRAIL_HOOK, CS_HOOK_PULL_SPEED, the hook model and six sounds, the level and client fields, the teleporter interactions, and the matching cgame rendering, impact effect and prediction input. Nothing in bg_pmove or the network protocol changes. Pm_CheckHook returns immediately unless pm.s.type is one of the PM_HOOK_* values, and only g_client.c ever set those, so the movement code is now unreachable in this module rather than deleted. pm_state's hook_position and hook_length stay networked for ctf's sake and simply remain zero here, which delta compression never sends. Two things deliberately survive, both developer tooling rather than gameplay: BUTTON_HOOK and the +hook command, which the AI nav editor uses to delete nodes, and cg_beam_hook, which despite its name is a generic rope sprite that also draws mover connections in that editor. default's game module now carries no hook symbols and no grapplehook assets; ctf keeps 36 and 7. Both still load and spawn 80 entities on edge, and the client still initializes. Refs #906
Fifteen conditionals asked whether team play was active by testing g_level.teams || g_level.ctf, because capture play deliberately turned teams off: "ctf overrides teams" forced g_level.teams to 0 whenever both were set. Every caller then had to remember that capture play was team play wearing a different flag. Invert it. Capture play now clamps g_level.teams on rather than off, so g_level.teams alone answers "are we playing teams" and g_level.ctf is left meaning only "are flags in play". The fifteen compound tests reduce to g_level.teams, and the clamp is applied wherever either value is assigned: the map, worldspawn and cvar precedence chain, and both cvar handlers. The create-server menu's capture entry set g_teams to 0 to match the old inversion; it now sets 1. Verified capture play on edge with g_teams explicitly 0: the clamp engages and the server starts clean.
g_entity_misc.c held the misc_teleporter, misc_fireball and friends, and the two copies differed only in the teleporter's grapple handling: detach a hook projectile at the mouth rather than let it anchor past it, and release a hooked player as they warp so they aren't tethered across the portal. Move it to src/game/common and wrap those two blocks in G_HOOK. A module opts in by adding GCOMMON_HOOK_CFLAGS to its compile flags, which is how the common sources learn the module has a hook at all; without it the references are never compiled and the module needs none of the hook's fields or symbols. Verified from the objects: the same source yields no G_HookDetach reference for default and one for ctf. This keeps the behaviour from #867 in ctf rather than trading it away for a shared file, and establishes the pattern for optional features in common code.
The hook was spread across five files of the ctf module, and the only way for a second mod to get it was to copy those files. Move it to src/game/common/g_hook.c, compiled only by modules that ask for it. Earlier I argued against this on the grounds that the hook needed 21 declarations from its host module. That counted the coupling as it stood rather than what it had to be. The hook now owns almost all of it: its eight cvars, its model and six sounds, its enabled state and the map's allowance are all private to g_hook.c, and g_hook_style_t moves to g_hook_types.h. G_CheckHook took g_level.ctf, a field common code cannot see, so G_Hook_CheckState takes the fallback as an argument instead. What a module must still supply is six lines: the types header, four wire values the client game reads off the network - CS_HOOK_PULL_SPEED, TE_HOOK_IMPACT, TRAIL_HOOK and MOD_HOOK - and two pieces of state, g_client_hook_t for the per-life hook and g_hook_style_t in the persistent client, which outlives respawns. default's g_types.h mentions the hook nowhere at all. G_ImmediateWall, a trace helper G_HookProjectile needed, was a static copy in each module's g_ballistics.c. It moves to g_effect.c, so there is now one definition rather than two. Verified: g_hook.c is compiled only for ctf, default's module has no hook symbols and no grapple assets against ctf's 64 and 7, and capture play on edge still spawns 85 entities with a clean log.
With techs, the grapple and the synthetic spawn hack already gone, this takes out the flags themselves: the four flag items and ITEM_TYPE_FLAG, the g_ctf and g_capture_limit cvars and their map and worldspawn precedence chains, CS_CTF, STAT_CAPTURES, SCORE_CTF_FLAG, the EF_CTF_* carrier effects, the three ctf sounds, pickup, drop, toss and dropped-flag return, the team flag classnames and flag entities, capture counters on teams, clients and the score wire struct, the bots' flag goals and carrier-priority weighting, and on the client the captures HUD counter, held-flag icon, scoreboard capture columns and carrier icon, and the carrier trail and shell effects. g_capture_t and PostStats stay as they are. They belong to src/game/game.h, the game import ABI shared with the engine and every module, so this module keeps the signature and passes NULL and 0. DEFAULT_TEAM_SKIN and the bot's "gork/ctf" skin stay too: despite the name, players/<model>/ctf.skin is the team-coloured skin used by all team play, and renaming it would mean renaming assets in quetoo-data. Three orphans went with it - Cg_OrbitalTrail and Cg_OrbitTrail_Think, which existed only to draw the carrier trail, and a stray return an earlier tech removal had left in the drop command. Verified the item table still aligns with the tag enum G_InitItems indexes directly, 49 slots for 48 entries plus the placeholder, and that the only items ctf now has which default lacks are the four flags and five techs. default's modules carry no flag or capture symbols. Deathmatch, team deathmatch and capture play all start clean on edge. Closes the Phase 2 strip for #906.
Moving the cgame UI and the shared game entities into common directories left the Xcode project referencing files that were no longer there, and adding a second game module broke it a second way: the project searched for headers recursively through $(SRCROOT)/src/**, so bg_item.h and g_types.h each resolved to two different files and a translation unit pulled in both, colliding on every item enumerator. File references only store a basename, resolving their path from the group they sit in, so relocating them is just re-parenting: the whole ui tree is one group and moves in a single step, alongside the twelve game files that went to common. The recursive search path is replaced by an explicit ordered list per target, which is what the autotools build always did with -I. This is a prerequisite for more than one module: two modules that both define g_types.h cannot share one recursive path. Verified with xcodebuild: the game and cgame schemes compile with zero source errors, having failed 13 and 19 respectively before. Linking still fails on Objectively.framework, which this project expects as a built sibling and which autotools instead resolves through pkg-config; that is unrelated to these changes and unbuilt in my environment. The ctf targets are not added yet. game-item and cgame-ui also need splitting per module before they can be: both are single prebuilt static libraries, and their sources include the module's own headers.
g_effect.c, which now holds G_Ripple and G_ImmediateWall, and cg_team_mode.c were created during the module split and never registered with the Xcode project, so the game target failed to link both symbols. Adds them along with the hook's files and cg_team_mode.h. g_hook.c is registered as a reference only, since it belongs to whichever targets opt into the hook rather than to the default module. Verified by building through Quetoo.xcworkspace, which resolves the Objectively frameworks from the sibling projects: the game, cgame and quetoo-dedicated schemes now build and link. The quetoo client scheme still fails in the renderer's Compile Shaders script phase for want of glslc, which autotools avoids by shipping the compiled shader blobs; it reports no source errors.
Four new targets, mirroring what autotools builds: game-ctf and cgame-ctf producing the module libraries, game-item-ctf for the module's own item table, and cgame-ui-ctf for the common UI. The two static libraries exist because their sources are not module agnostic. bg_item.c includes g_types.h and the UI sources include cg_local.h, so each module needs its own objects compiled against its own headers - the same reason autotools builds libcguidefault alongside libcguictf. game-pmove is shared, since bg_pmove.c includes only its own header. The rest of the common sources are compiled straight into each module target rather than through another library. Both modules produce files named game.so and cgame.so, so the ctf targets set CONFIGURATION_BUILD_DIR to a ctf subdirectory of the products directory, which matches the layout the engine expects under lib/quetoo. That redirect also moves where the linker looks, so their LIBRARY_SEARCH_PATHS names the parent products directory to find game-pmove and discord-rpc. Static library targets keep an empty Frameworks phase, as libtool -static rejects -l flags. game-ctf defines G_HOOK, which is what pulls g_hook.c and the teleporter's hook handling into the build. Verified against Quetoo.xcworkspace: the game, cgame, game-ctf, cgame-ctf and quetoo-dedicated schemes all build and link. The products land flat for default and under ctf/ for ctf, and carry the right contents - default's game.so has no tech, flag or grapple symbols and no such asset strings, retaining only Pm_CheckHook and Pm_CheckHookJump, which are unreachable without a module setting pm.s.type. Autotools still builds clean.
The AI was the largest duplicated block left: 5044 lines across six sources and seven headers, of which four sources and every header were byte-identical between the two modules. The only real divergence was 44 lines - the bots' interest in techs and flags, which the default module lost when capture play was removed from it. Move all of it to src/game/common and guard those four blocks with G_CTF: the tech and flag goal filters in g_ai_item.c, and the flag-carrier target priority and chase weighting in g_ai_main.c. A module opts in with GCOMMON_CTF_CFLAGS, alongside GCOMMON_HOOK_CFLAGS for the grapple. A dead commented-out hook line went too. Nothing in the AI needed abstracting. Its remaining CTF-adjacent details are asset names rather than gameplay: g_ai_info.c names a "gork/ctf" bot skin, which is the team-coloured skin all team play uses, and g_ai_node.c reads BUTTON_HOOK as the nav editor's delete-node input, which stays defined for every module in the common bg_pmove.h. In Xcode the AI references move to the common group and the ctf targets' duplicates are dropped, their build files repointed at the shared references, with G_CTF added to game-ctf. Verified both ways: autotools produces distinct objects per module, ctf's g_ai_main.o being 1624 bytes larger for the flag logic, and both modules load the same 346 nav nodes on edge with clean logs. The game, game-ctf, cgame and cgame-ctf schemes all build, and each module's product carries 214 AI symbols while only ctf's names flag and tech assets.
The cgame was the last large duplicate: 14387 lines, of which roughly 6700 were already byte-identical between the two modules and the rest differed by only about 350 lines. Move all of it to src/cgame/common, leaving each module just its team mode table. The differences fell into three kinds. Four files differed only by a module-qualified include, game/default/bg_item.h against game/ctf's; those become bare includes resolved through a per-module -I, so the divergence disappears rather than needing a guard. The rest is grapple code behind G_HOOK - the trail, impact effect, prediction input, cvar and sample - and capture-play code behind G_CTF: the captures counter and held-flag icon, the scoreboard columns and carrier icon, the carrier trail and shell, and the Discord mode label. The guards were derived from the difference between the two copies, then checked both ways: dropping the guarded lines must reproduce default's file and dropping only the directives must reproduce ctf's. That caught four files where a hunk boundary fell inside a doc comment, so the opening /** swallowed the #endif, and one where an #endif landed between } and else. Those are guarded by hand around whole functions and statements instead, and a check now asserts no directive sits inside a comment. cg_types.h stays common but keeps resolving g_types.h per module, so cg_state_t still takes each module's definitions - which is why these sources are compiled per module rather than linked from one library. Verified both build systems. Every guarded object is larger for ctf than for default, by 104 to 10176 bytes, and the capture strings appear only in ctf's module. All seven Xcode schemes build, autotools builds clean, and the client still initializes.
Sharing cg_hud.c and cg_score.c behind G_CTF was the wrong call. It put mod knowledge into common code, and it would only accumulate: a fifth module wanting lap times adds G_RACE guards to the same shared file, and common ends up knowing about every mod that will ever exist. The HUD and scoreboard are the most visible thing a mod changes - they are content, not mechanism, the same category as bg_item.c. So both files go back per module, with no guards. What they share instead is cg_hud_draw, the drawing primitives that know nothing about which stats a module shows or how it arranges them: the crosshair, screen blends, center print, target name, the weapon bar, and the icon, vital and powerup helpers, along with the HUD state and media they own. A module's HUD is now composition only, 356 lines for default and 434 for ctf against 840 shared, rather than 1300 duplicated. The alternative was about six hook functions - draw an extra badge, an extra column, an extra row - which is more API surface than the duplication saves and freezes the layout besides: a module could append elements but never restructure. Primitives leave each module free to arrange its own HUD while generic fixes still flow from common. cg_hud_state and the blend and weapon-bar media move to common with the primitives that own them, since that is where nearly all their use is: 59 references against 6. Verified both build systems, and that Xcode compiled the per-module files once rather than twice after the references moved. All seven schemes build, autotools is clean, the client initializes, and only ctf's module carries the capture strings.
Five more pairs move to src/game/common: the weapon projectiles, the trigger_* entities, the client view, the chasecam and the info_* entities. All five were byte-identical between the modules, and none contains a single gameplay-mode branch. g_ballistics was the judgement call, since weapons are what mods change and by that reading it is content. But it has no mode branches at all and reads its four g_balance_* cvars from each module's own g_main.c, so the tuning is already the module's and the projectile code is mechanism. A module that wants genuinely different projectiles overrides the file. g_entity_trigger diverged by eight lines - what becomes of a dropped item that falls into the void. Guarding that with G_CTF would have repeated the mistake of teaching common code about particular modules, so it becomes a seam instead: G_ResetDroppedItem, declared in the new g_module.h and implemented per module. Default frees the item; ctf returns flags to base and respawns techs. G_Explode in g_util.c had the same dispatch and now shares the seam, which also cleared two orphaned break statements an earlier strip had left in default's copy. A game module is now 11612 lines for default and 12721 for ctf, from 14273 and 15381. Verified both build systems: the seam resolves as one definition per module with the common object referencing it, all seven Xcode schemes build, autotools is clean, and capture play on edge still spawns 85 entities against deathmatch's 80.
g_util.c held 26 functions of which only four were capture-specific: G_TeamForFlag, G_FlagForTeam, G_EffectForTeam and G_GetFlag. The other 22 - entity lookup and allocation, targets, kill boxes, explosions, gibs, the terrain predicates, animation, and the gameplay and team name helpers - were identical between the modules. Move those to src/game/common and give ctf the four flag helpers as its own g_flag.c. Nothing needed guarding: the divergence was whole functions belonging to one module, so they simply move there. A game module is now 10854 lines for default and 12016 for ctf, and a cgame module 768 and 871. Verified all seven Xcode schemes, autotools, and both modules at runtime: deathmatch spawns 80 entities on edge and capture play 85, both clean.
Building the client regenerated all 27 .dxil blobs on any machine whose shaderc differs from the one that produced the committed copies, dirtying the tree every time. Only DXIL was affected; .spv and .metal reproduce byte for byte. D3D12 is unusable pending an SDL_gpu fix, so only Vulkan and Metal are exercised and the DXIL output has no consumer today. Drop the four transpilation steps that produce it. The committed blobs stay, and stay installed: ObjectivelyGPU requests all four shader formats, so SDL is free to hand back a D3D12 device, and its loader resolves .dxil for one. Deleting them would leave that path unable to load shaders at all rather than merely hitting the SDL_gpu bug. Removing them wants DXIL dropped from the requested formats too, which is a change in ObjectivelyGPU rather than here. Also ignore /build/, which xcodebuild -project leaves at the repository root; building through Quetoo.xcworkspace uses DerivedData instead. Verified the tree stays clean through a full client build.
Xcode rewrote the hand-authored entries into its own formatting when it first built these targets, and generated shared schemes for the four new ones. Both are worth keeping: the schemes make game-ctf, cgame-ctf, game-item-ctf and cgame-ui-ctf selectable in the UI and buildable with xcodebuild -scheme, and the normalized file is what Xcode will write anyway. Verified the four module schemes still build after the rewrite.
The controls menu offered a Grapple hook section to every module, but default has no grapple: cg_main.c already guards hook_style behind G_HOOK, so the CvarSelect bound to a cvar that does not exist and logged a "not found" warning. Guard the section the same way, identifying the box so a module without the feature drops it. +hook itself stays registered for all modules — it is the navigation editor's delete-node input, and the engine binds it to mouse 2 by default, so nothing is stranded by losing the menu row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Splitting the game and cgame sources into src/game/common and src/cgame/common left Quetoo.vs15 behind: 116 of cgame.vcxproj's 134 sources, 35 of game.vcxproj's 54, libpmove's bg_pmove and nine QuetooFullIncludePath entries all pointed at files that had moved. The common file lists now live in game_common.props and cgame_common.props, imported by each module's project, so the four projects share one list the way the autotools builds share sources.mk. A module's own directory goes in its project's AdditionalIncludeDirectories rather than in the global include path, where two modules' g_types.h and cg_local.h would be ambiguous. game-ctf and cgame-ctf mirror their Xcode targets: they keep the names the engine loads them by, build into a ctf subdirectory so they do not clobber default's output, and define G_HOOK and G_CTF. ROBO recurses, so the module copies now use a flat variant that cannot drag one module's output into the other's install directory. Unlike autotools and Xcode, none of this is build verified; it needs a Windows toolchain to confirm. Refs #906 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The filters files had drifted from the projects they group: libnet still listed net.c and net_tcp.c, replaced by net_sock.c, and cmodel, server, shared and quemap were each missing a pair of sources added since anyone last opened the solution. Being IDE grouping only, none of this broke a build, which is why it went unnoticed.
The rebase carried func_bob into src/game/common/g_entity_func.c along with the rename, but ctf's own g_types.h, g_entity.c and g_client.c were branched from default before func_bob existed, so the module was missing the three per-module halves: MOD_BOB, the entity class registration and the obituary. Common code referencing MOD_BOB meant ctf failed to compile, which is the contract working as intended. MOD_BOB joins the names sources.mk records as contract between the common sources and every module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jdolan
commented
Aug 5, 2026
jdolan
commented
Aug 5, 2026
jdolan
commented
Aug 5, 2026
jdolan
commented
Aug 5, 2026
jdolan
commented
Aug 5, 2026
| /** | ||
| * @brief Detach the player's hook if it's still attached. | ||
| */ | ||
| void G_HookDetach(g_client_t *cl) { |
Owner
Author
There was a problem hiding this comment.
Disabling the hook mid-level strands a client who is attached at that moment.
G_CheckCvars_Hook returns super.CheckCvars() without requesting a restart, unlike G_CheckCvars_Tech which sets restart = true. Both G_HookDetach here and G_HookThink return immediately when !g_hook_enabled.
So a client pulling on the hook when an admin sets g_hook 0: g_hook_enabled goes false, no restart happens, G_HookThink never runs again, and G_HookDetach refuses to act — cl->hook.pull stays true, so G_PrepareMove_Hook keeps forcing PM_HOOK_PULL and the projectile and trail entities keep thinking, until the client dies or the map changes.
The early-return is what makes it unrecoverable rather than merely odd. Either tear down an already-attached hook before returning, or set restart = true as the techs do and let the level restart clear it.
jdolan
commented
Aug 5, 2026
jdolan
commented
Aug 5, 2026
| import.Draw3DBox = R_Draw3DBox; | ||
|
|
||
| cgame_handle = Sys_OpenLibrary("cgame", true); | ||
| cgame_handle = Sys_OpenLibrary("cgame"); |
Owner
Author
There was a problem hiding this comment.
A failed Cl_InitCgame leaks cgame_handle and leaves a stale addClassImage registration.
If Sys_OpenLibrary succeeds and addClassImage(cgame_handle) runs, but Sys_LoadLibrary then errors — an unresolved Cg_LoadCgame, or the api_version mismatch below — cls.cgame stays NULL. On the next Cl_InitCgame, the if (cls.cgame) guard is false, so Cl_ShutdownCgame is skipped: the previous dlopen handle is overwritten and never closed, and its class image is never removeClassImaged, so classForName keeps a registered image nobody owns.
MAX_CLASS_IMAGES is 8 with an assert, so repeated failed loads eventually abort. This matters more now than it would have before, because dropping RTLD_GLOBAL makes the addClassImage/removeClassImage pairing load-bearing rather than incidental.
Suggest tracking and cleaning cgame_handle independently of cls.cgame — unregister and close at the top of Cl_InitCgame, or on each error path.
Owner
Author
There was a problem hiding this comment.
Still open, but narrowed. The game command pre-checks Sys_HasLibrary before it tears anything down, so the most reachable cause — asking for a game that ships no cgame — no longer reaches this path. What remains is a failure after dlopen succeeds: an unresolved Cg_LoadCgame, or the api_version mismatch below. Those still leave cgame_handle open and its class image registered, with cls.cgame NULL so the next Cl_InitCgame skips the shutdown.
Worth tracking separately rather than holding the branch.
Owner
Author
Review: 362 files, five reviewers, five areasReviewed the engine, the game common seams, the cgame/HUD split, the three build systems, and cross-module divergence. Verification I ran myself, not just read:
The architecture holds up. The mechanism/content split, the chainable hooks with their Fixed and pushed (7 commits)
Raised inline, not fixed — these are yours to callTwo I would want resolved before merge:
Also raised: the cgame hot-swap while connected to a remote server ( Three more that had no diff line to attach to:
Checked and cleanPer-module item invariants hold in all three modules (roster/enum/tag order agree within each; stat counts under One thing worth recording: the removals of the per-map Nothing I found argues against merging today once (1) and (2) are settled. |
DropInventoryItem handed a name down the chain, so a feature answering for a category had to name the item it meant. All four flags are called "Enemy Flag", and the tail resolves a name with G_FindItem, which returns the first available match - always FLAG_RED. A player carrying the blue flag typed "drop flag" and was told "Out of item: Enemy Flag"; only the red flag carrier could drop at all. Make the chain a resolver: ResolveInventoryItem returns the g_item_t a name means, and ctf and the techs return the item being carried rather than its name. Dropping is then a plain function over a resolved item, and G_Drop_f owns the one message that needs the string the client typed. The techs escaped this only because their five names happen to be unique. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sys_OpenLibrary resolves cgame.so through the whole search path, and lib/quetoo/default is a retained base path, so a client without the server's module quietly loaded default/cgame.so instead. Nothing caught it: the game cvar and cls.cgame_game record the module that was asked for, not the one that was found, and all three modules currently advertise the same protocol minor. The client then read the server's stats against the wrong layout - ctf inserts STAT_CAPTURES and STAT_TECH, shifting every later index. Have the module answer for itself. cg_export_t carries the GAME_NAME its own g_types.h defines, so one line in common gives each module its own, and Cl_ParseServerData drops when it disagrees with the game directory the server advertised. Bump CGAME_API_VERSION, because the new field shifts every function pointer after it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sys_OpenLibrary accepted whatever the search path resolved game.so and cgame.so to. Every module carries those same two names, and <lib_dir>/default stays mounted as a base path for the shared UI, so a client or server asked for a module it does not have silently loaded default's image and ran deathmatch under the mod's name - with the mod's stats layout expected on the wire. Directories are prepended as they are mounted and Fs_SetGame mounts the game's own last, so the game's copy already wins wherever it exists. Resolving to any other directory therefore means this game has no module, which makes the whole check one comparison against the resolved directory's leaf. Should that ordering ever change, the module is refused rather than silently wrong. A missing module is now a missing module: Error: Sys_OpenLibrary: Couldn't find game.so for game nosuchmod Verified against the installed tree: default, ctf and lithium each load from their own directory, spawning 80, 85 and 85 entities on edge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jdolan
force-pushed
the
feature/mod-support
branch
from
August 5, 2026 21:50
b1ceb61 to
567d33b
Compare
The closing comment restated the macro immediately above it in 106 headers, which is noise the compiler already checks.
The game was a CVAR_LATCH cvar, which conflated two different facts: what the user asked for, and what is currently mounted. Seventeen sites touched it, three of them calling Com_SetGame under different preconditions, and two frames polling its modified flag - one to remount the filesystem, the other to reload the client game, neither knowing whether the other should have run. That split is what let a game change apply mid-session. CVAR_LATCH defers only while QUETOO_SERVER is set, and that bit means a map is loaded locally, so a client connected to a remote server took the immediate branch: the filesystem was remounted and the client game swapped underneath a live connection, with no media load and no protocol recheck. Changing games is an operation, not an assignment, so it is a command: game reports the game that is current game <name> validates the name, brings any session down, and switches +game <name> the startup game, replacing +set game <name> quetoo_t::game holds the state and Com_Game() reads it. Com_SetGame is the only writer and carries no policy; the command owns the policy, because main.c is the only place that knows about both the server and the client. The cvar is gone: nothing parsed it back out of server info, and the readers that wanted it wanted the current game, which Com_Game() now answers. Sys_HasLibrary shares Sys_OpenLibrary's resolver so a game that ships no module is refused before anything is torn down, and a mistyped +game falls back to default rather than failing inside the load. Verified against the installed tree: +game ctf and +game lithium each load their own module and spawn 85 entities on edge against default's 80; a bare game prints the current one; ../../x is refused; nosuchmod is refused and rolled back; and a bare +game followed by +map no longer reads +map as the game name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A game is a directory under the library directory that ships a client game. Only the game that is current is mounted, so Fs_CompleteGame reads the real filesystem rather than the search path, and globs cgame.* rather than naming it, leaving the shared library extension to the platform. A directory with no module is not offered, which is the same question the game command asks before it tears anything down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Fs_Game() had no callers left once Com_Game() became the source of truth, and Fs_SetGame has exactly one caller, Com_SetGame, which holds the game that is current and returns early when it is unchanged. So the filesystem was keeping a second copy of the state purely to answer a question nobody asked and to repeat a comparison its only caller had already made. Fs_SetGame keeps validating its argument: that guards a public function against its next caller, rather than tracking state. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The per-map gameplay settings, G_CreateTeamSpawnPoints and the g_ctf cvar were deliberate deletions, but they read as regressions from the diff alone - two reviewers reported them as bugs, and the reasoning was only in a commit message. Also record what the loader now guarantees: a module is resolved only from the directories that are the game that is current, so a mod that ships no cgame is refused rather than substituted, and the client checks the GAME_NAME the loaded image advertises against the game the server sent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 6, 2026
Owner
Author
Review closed outResolved on the branch since the review, with the inline threads resolved:
Carried forward as issues, none of which blocks this branch:
Verification. All six module targets build clean with zero warnings; Not verified by me: anything requiring the GUI client. Every runtime check above drove |
Owner
Author
Correction to the review summary aboveI called #918 the one thing to take before merging. That was wrong, and @jdolan caught it: So there is no outstanding merge blocker from this review. The carried-forward list is now:
The only thing I could not check myself remains the GUI client: |
CS_BSP_SIZE was removed in favour of the map manifest, but maps/<name>.mf is read from the client's own filesystem and Cl_CheckOrDownloadFile skips the download when the file exists, so a locally consistent but stale pair passed: client on edge v1, server on edge v2, no download, hashes agree locally, and the client predicting against a different collision model. The check that was dropped was the only server-authoritative one. Put it back as a hash rather than a size, which is what the check was always reaching for - a size passes for same-size different-content. The server hashes the bsp it actually loaded rather than quoting its own manifest, so a stale .mf can not have it reject correct clients, and both sides go through Cm_HashFile so they compute it the same way. Verified against edge: quemap's manifest, md5(1) and Cm_HashFile all agree on 3d888515a237519027d9bab1507dbc77. It reclaims the config string slot CS_BSP_SIZE vacated, so CS_MODELS returns to 5 and the numbering matches main again. PROTOCOL_MAJOR was already bumped on this branch for that shift, so this costs no further break - which is why it is worth doing before the merge rather than after. The manifest check drops its fatal case for the bsp with this. It compared our own file against what our own file claims, so it could only ever report a locally inconsistent pair, and it fired for the host of a listen server, where client and server are one process reading one file. It warns like every other asset now, and no longer errors out from inside the enumeration it was leaking. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements #906.
The game module loading race is fixed,
ctfbecomes a game and cgame module of its own, and the code both modules inherit moves intosrc/game/commonandsrc/cgame/common.Shape
A mod is its own gameplay, not a fork of the engine's furniture. The common directories hold what every module inherits; each module owns the rest.
Common sources are compiled per module, never linked from one prebuilt library. They resolve
g_types.h/cg_local.hfrom the compiling module's directory, so a single prebuilt copy would bake one module's struct layout into all of them. The concrete hazard:cg_state_tembedscg_team_info_t teams[MAX_TEAMS]plusg_gameplay_tandg_items_t, all from the per-moduleg_types.h, so any mod changing its team roster or item set would readcg_stateat the wrong offsets.Mechanism is shared; content stays per module. The tell is a gameplay-mode branch —
g_level.gameplay,g_level.teams,GAME_INSTAGIB. Zero branches means mechanism. By that ruleg_ballistics.cis shared despite being weapon code, whilebg_item.c,g_item.c,g_main.c,g_combat.c,g_weapon.c,g_client.c,g_entity.c,g_types.hand the client'scg_hud.c,cg_score.c,cg_team_mode.cstay per module.A named per-module function beats an
#ifguard. A guard teaches common code about every module that will ever exist. The seams areG_ResetDroppedItem(src/game/common/g_module.h) andCg_TeamModes(src/cgame/common/cg_team_mode.h). Guards are reserved for optional features a module opts into wholesale:G_HOOKfor the grapple,G_CTFfor capture play.Where only presentation varies, share primitives rather than adding hooks.
common/cg_hud_draw.cholds the crosshair, blends, center print, weapon bar and icon/vital/powerup helpers; each module'scg_hud.cis composition only. Hooks were rejected here — roughly six would have been needed, and they freeze the layout so a module could append elements but never restructure.A module can always override a common file by substituting its own in its source list. That escape hatch is what makes sharing safe.
Build systems
All three are updated. Autotools and Xcode are build verified.
Quetoo.vs15needed repair as well as extension: the file moves had left 116 ofcgame.vcxproj's 134 sources and 35 ofgame.vcxproj's 54 pointing at files that no longer existed. The common lists now live ingame_common.props/cgame_common.props, imported by all four module projects, mirroring how the autotools builds sharesources.mk. A module's own directory goes in its project'sAdditionalIncludeDirectoriesrather than the global include path, where two modules'g_types.handcg_local.hwould be ambiguous.Verification
makefrom scratch, exit 0, no new warnings; all four modules link.Quetoo.xcworkspace(not-project, so the Objectively frameworks resolve from the sibling checkouts).quetoo-dedicated +set game <module> +map edge +quit) is not re-run since the rebase; it needs an install, as modules load fromPKGLIBDIR.Notes for review
origin/main, which brought infunc_bob. The rename carried it intosrc/game/common/g_entity_func.c, but ctf's owng_types.h,g_entity.candg_client.cpredated it, so the last commit gives ctf itsMOD_BOB, class registration and obituary. Common code referencingMOD_BOBmade ctf fail to compile until then — the contract working as intended.DEFAULT_TEAM_SKINand the bot'sgork/ctfskin are the team-coloured player skin all team play uses, andcg_beam_hookis a genericsprites/ropebeam also used by the AI nav editor.BUTTON_HOOKlikewise survives indefaultas that editor's delete-node input.🤖 Generated with Claude Code