External weapon model refactor: consolidate per-bank state, fix design issues - #7634
Open
Goober5000 wants to merge 5 commits into
Open
External weapon model refactor: consolidate per-bank state, fix design issues#7634Goober5000 wants to merge 5 commits into
Goober5000 wants to merge 5 commits into
Conversation
Goober5000
added
fix
The state for a bank's external weapon model was scattered across two structs and three naming schemes: the model instance and its weapon in ship_weapon, the firing-point chain counter in a combined array indexed with the bank + MAX_SHIP_PRIMARY_BANKS packing convention, and the gatling spin rate/angle on ship itself. Gather them into external_weapon_state, with one array for primary banks and one for secondary banks in ship_weapon. This removes the index-packing convention (ship_get_external_model_fp_offset now takes the bank's state directly), and ship_delete now also frees secondary instances should a future weapon ever create one. No behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The gatling spin state was integrated inside ship_fire_primary (spin-up) and ship_stop_fire_primary_bank (spin-down), which entangled a per-frame visual/state update with the firing event paths and had two quirks: - Spin-down only ran when something called the stop-fire path, so a bank could freeze mid-spin (e.g. AI ships that stop calling ship_fire_primary without going through ship_stop_fire_primary). - The spin angle only advanced when the ship class set $Show Primary Models:, so identical weapon state evolved differently per ship class even though the rate (which gates firing) always ran. Banks that try to fire now just request spin-up, and update_external_weapon_spin() -- called from ship_process_post next to update_reload_percent(), which was moved out of the physics code for the same reason -- integrates the rate and angle every frame. The firing gate (no shots until the barrels reach full speed) is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…wn models Since the original 2005 implementation, external_model_num was bashed to model_num at parse time when no $External Model File: was given, so every POF-rendered weapon appeared to have an external model. All the checks downstream therefore tested 'has any model' while reading as 'has an external model', and two paths leaked the fallback into gameplay: - ship_fire_secondary applied the external-model firing point offset without checking $Show Secondary Models: (unlike the primary path), so any missile whose own in-flight POF contained gun points would have its launch position offset on every ship in the game. - The AI line-of-sight check, copied from ship_fire_secondary, applied the offset with no draw check for either bank type, so for primaries it could disagree with the actual firing path. external_model_num now stays -1 unless $External Model File: was given. Rendering code (ship_render_weapon_models, the HUD hardpoints gauge, and the gatling instance helper) falls back to the weapon's own model at the point of use, preserving the existing display of missiles on hardpoints. Firing point offsets now require a dedicated external model *and* the bank's models to actually be drawn, in all three paths. In practice this does not change firing behavior, because GPNT chunks are never added to weapon models except when explicitly built as external models. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two load-bearing conventions of the external weapon model feature had no diagnostics: - Only the first gun bank of an external model supplies firing points; additional banks are silently ignored. Warn when an external model has more than one gun bank. - $Show Primary Models: / $Show Secondary Models: must list exactly one bool per weapon bank; missing entries silently default to not drawing. Warn when the list length doesn't match the bank count. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
External weapon models were always rendered pointing ship-forward, and reloading missiles slid backward along fixed -Z, regardless of the gun or missile point's normal -- so a bank angled 45 degrees outward fired along its normal while its mounted model pointed dead ahead. The per-slot angle offset was also applied as a roll about ship-forward rather than about the firing axis. ship_get_weapon_model_slot_transform now builds the slot orientation from the firing normal (identity for the usual dead-ahead normal, so existing forward-facing mounts render exactly as before) with the angle-offset roll applied about that axis, and slides reloading missiles along the normal. Correspondingly, ship_get_external_model_fp_offset now rotates the external model's firing-point offset by the same slot transform, so shots continue to emerge from the drawn barrels on angled mounts, and now also respect the angle-offset roll (previously the offset was added unrotated, mismatching rolled models). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Goober5000
force-pushed
the
refactor/external_weapons
branch
from
July 21, 2026 04:16
cfa76cc to
119ff47
Compare
Kestrellius
reviewed
Aug 20, 2026
|
|
||
| Assertion(sub_shot >= 0 && sub_shot < bank.num_slots, "sub_shot %d is out of range for the external model of weapon %s, which has %d firing points", sub_shot, wip->name, bank.num_slots); | ||
| return bank.pnt[sub_shot]; | ||
| vec3d slot_pnt, offset; |
Contributor
There was a problem hiding this comment.
Do I understand correctly that slot_pnt is here purely to satisfy the parameters of ship_get_weapon_model_slot_transform()?
Kestrellius
reviewed
Aug 20, 2026
| dir = pm->gun_banks[bank_to_fire].norm[pt]; | ||
|
|
||
| vec3d external_fp_offset = ship_get_external_model_fp_offset(swp, winfo_p, weapon_model, bank_to_fire, true, s); | ||
| vec3d external_fp_offset = ship_get_external_model_fp_offset(&swp->primary_bank_external_weapon[bank_to_fire], winfo_p, weapon_model, &pm->gun_banks[bank_to_fire], pt, true, s); |
Contributor
There was a problem hiding this comment.
We're doing the same vector math afterward in all three places this function is called, so you might consider passing in the shipobject or its orientation so you can just handle it inside. It'd be a more invasive change to the function though.
Kestrellius
reviewed
Aug 20, 2026
Kestrellius
left a comment
Kestrellius
left a comment
Contributor
There was a problem hiding this comment.
On the whole, looks good. Definitely much cleaner than the previous state.
A couple things to note.
On master, the rotation update step happens before we try to fire the primaries (for the player. For AI, this appears to be true iff Framerate_independent_turning is off.) Moving it from obj_move_all_call_physics() to obj_move_all_post means that it now happens after we try to fire the primaries, which means that the firing of the player's spoolup weapons will be delayed by one frame compared to master. I don't think this is much of a problem, but I want to make sure it's been explicitly thought about before I approve.
Another possible behavior change: on master the deceleration when not firing happens in ship_stop_fire_primary_bank(). That function and ship_stop_fire_primary() get called in a lot of places, and I suspect but am not sure that it's possible for more than one of those calls to trigger for the same bank within the same frame, in which case the external model would decelerate twice and rotate twice. In the new version the deceleration and rotation definitely only happens once per frame. But that's obviously the more correct behavior, so if this is an actual change it's just a bugfix.
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
2 participants
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.
Follow-up to #7588, laying the groundwork for future fixes and features.
Consolidate per-bank state into one struct. The state for a bank's external weapon model was scattered across two structs and three naming schemes. These are now gathered into an
external_weapon_statestruct, with one array for primary banks and one for secondary banks.Move gatling spin into per-frame processing. Spin-up/down was integrated inside
ship_fire_primary/ship_stop_fire_primary_bank, entangling a per-frame visual update with the firing event paths. Firing now just requests spin-up, and a newupdate_external_weapon_spin()inship_process_postintegrates rate and angle every frame.Make
external_model_nummean what it says. Since the original 2005 implementation,external_model_numwas bashed tomodel_numat parse time when no$External Model File:was given, so every POF weapon appeared to have an external model.Orient mounted models along the firing normal. External models now point where they should, in the rare case that the firing normal is not straight ahead.
Add diagnostics for silently-ignored conventions. Warn when an external model has more than one gun bank (only the first supplies firing points), and when a
$Show Primary/Secondary Models:list length doesn't match the bankcount (missing entries silently defaulted to not-drawing).