Skip to content

Honour useMuzzle in projectile weapons' GetAimFromPos - #3317

Draft
keithharvey wants to merge 2 commits into
beyond-all-reason:masterfrom
keithharvey:weapon-aimfrom-prefire
Draft

Honour useMuzzle in projectile weapons' GetAimFromPos#3317
keithharvey wants to merge 2 commits into
beyond-all-reason:masterfrom
keithharvey:weapon-aimfrom-prefire

Conversation

@keithharvey

@keithharvey keithharvey commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Work

CWeapon::TryTarget calls GetAimFromPos(preFire) to pick the point it tests line of fire from: the aim-from piece while deciding whether a target is worth taking, the muzzle when actually firing. CMissileLauncher, CCannon and CStarburstLauncher override it to return the muzzle unconditionally, so for them the pre-fire test runs from wherever the muzzle piece happens to rest. This makes the three overrides honour useMuzzle, which is what the base class already does.

TestRange measures from aimFromPos directly and is unchanged. The only callers of GetAimFromPos are TryTarget and Lua's GetUnitWeaponHaveFreeLineOfFire default source. Firing still needs a clear line from the muzzle, since UpdateFire tests with preFire = true.

Issue

Beyond All Reason's Rocketeer rests its launcher arm hanging down, and the arm carries the muzzle piece. From that pose any ridge between the bot and a target blocks the muzzle ray, Attack refuses the target, and because a weapon is only aimed at a target it has accepted, nothing ever raises the arm. Game-side set targets stay passive forever, attack orders make the bot path around the obstacle, and auto-targeting never fires. One shot at the floor unjams it until the arm drops again. Reported and diagnosed on the BAR Discord; the game-side workaround is beyond-all-reason/Beyond-All-Reason#9056.

Test

Built spring-headless from this branch and ran BAR's ridge scenario (Rocketeer at rest behind a 9-elmo ridge, target 360 elmos away) on stock game code with the workaround removed: ground Set Target accepted in 1 frame, unit Set Target in 1 frame, Attack order in 1 frame without moving, fire-at-will enemy in 9 frames. On 2026.07.04 none of the four is ever accepted. Line of fire measured from the aim piece and from the muzzle piece at rest agree with the new default and the old one respectively.

Behaviour note: a cannon whose barrel tip is blocked while its turret pivot is not will now accept the target and hold fire until the line clears, where before it would have refused the target and, under an attack order, moved closer. That is already how every non-projectile weapon behaves.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Y8JZx1GnqtfY8VUXjLZTkx

CWeapon::TryTarget asks GetAimFromPos(preFire) for the point to test
line of fire from: the aim-from piece while deciding whether a target
is worth taking, the muzzle when actually firing. CMissileLauncher,
CCannon and CStarburstLauncher override it to return the muzzle for
both, so a unit whose muzzle piece rests somewhere it cannot see from
(Beyond All Reason's Rocketeer rests its launcher arm hanging down)
refuses every target behind a bump, and since a weapon is only aimed
at a target it has accepted, nothing ever raises the muzzle. Set
targets go passive, attack orders make the unit path around the
obstacle, and auto-targeting never fires at all.

Return the aim-from position unless the caller asked for the muzzle,
as the base class does. TestRange already measures from the aim-from
position, so this only changes the pre-fire line of fire test and
Lua's GetUnitWeaponHaveFreeLineOfFire default source. Firing still
requires a clear line from the muzzle.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Y8JZx1GnqtfY8VUXjLZTkx
@sprunk

sprunk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

7ab7665 explicitly left these weapon types out, figure out why.

@eun-ice

eun-ice commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

I have locally verified: arm rocket bots can shoot over walls with this change.

There was a friendly fire issue in beyond-all-reason/Beyond-All-Reason#550 that was fixed by lowering the aim point so rockos don't shoot at eachother. We should reverify, maybe the later introduced fastQueryPointUpdate=true might fix the friendly fire?

Also check 344ecd7 where the commit message explicitly states we should later do the change @keithharvey is doing here.

@keithharvey

Copy link
Copy Markdown
Contributor Author

Yeah I also verified this in headless @eun-ice.

@sprunk 344ecd7 (2016) made the base class use aimFromPos before firing and the muzzle for the last check, and left the overriding weapons on the muzzle "to keep them as is for this release", noting the alternation would be useful for them later. 7ab7665 (2017) then refactored the LOF signatures and added these GetAimFromPos overrides to preserve that behaviour through the refactor. I can't find a reason beyond that. The fire-time check is unchanged and still runs from the muzzle.

@sprunk

sprunk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Headless doesn't test anything. Do a proper test in an actual edge case scenario where the aimpoint difference might matter, e.g. setup something like this one https://www.youtube.com/watch?v=jR1H8U6IjFo

@sprunk

sprunk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

The core of that setup is something like this, with rapid jittery movement of the shooter, and some movement of the target to catch various tight angles. Right now this works (as per the video above), make sure it doesn't break it.
image

If a test like the above works (i.e. the unit doesn't hit the obstacles) then the PR sounds fine.

@eun-ice

eun-ice commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Headless doesn't test anything. Do a proper test in an actual edge case scenario where the aimpoint difference might matter, e.g. setup something like this one https://www.youtube.com/watch?v=jR1H8U6IjFo

I do not really understand the video, is this testing the same case as me manually in the video beyond-all-reason/Beyond-All-Reason#9055 ?

@sprunk

sprunk commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

It's similar but yours is very static and "easy" for the unit, there aren't tight angles involved. In the video the shooter has a large discrepancy between the muzzle and aimfrom (the long gun), which is the main thing I worry about. There's also rapid movement, the gap to shoot through is pretty small, there's technically a ballistic weapon, and the target is moving (adding shot leading), which are all meant to produce shots adjacent to the allied units. The shooting unit is neither afraid of shooting nor ever hits the allies despite all that. Basically, the video is much more thorough at testing the worrisome edge cases of friendly fire due to using the wrong piece, compared to just having a vanilla rocko shoot once over dragons teeth. Maybe @GoogleFrog could add if I'm missing something.

@eun-ice

eun-ice commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Stylistic question, wouldreturn CWeapon::GetAimFromPos(useMuzzle); be a better choice? Or do we want return (useMuzzle ? weaponMuzzlePos : aimFromPos) incl. the extra parenthesis?

They are equivalent, I know, but I want to ask about stylistic preferences of this project.

@eun-ice

eun-ice commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

@sprunk is this what you consider a reproducer? this is master, working on making the same with the PR and different unit types so we have different heights of target and friendly units?

armrock-friendly-fire-master.mp4

@eun-ice

eun-ice commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

With this PR the identical scenario results in one more shot being fired, but still 0 friendly fire:

armrock-friendly-fire-pr3317.mp4

I do not think friendly fire is going to be changed with this PR, as during prefire the engine checks TryTarget(..., true) (which sets preFire true) in Weapon.cpp:475, which then does GetAimFromPos(preFire), so "useMuzzle" is true and that path is not changed by this PR

@eun-ice

eun-ice commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Here is a situation where this PR doesnt shoot (on the third round)

armrock-mace-pr3317.mp4

but on master it would shoot

armrock-mace-master.mp4

In the instance where master shoots it uses muzzlePos: y=198.19, in PR it uses aimFromPos: y=195.10

So we have to adjust aimFromPos higher I think.

@eun-ice

eun-ice commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

My tests reveal an actual issue with this change: in cases where the unit does have a line of fire with muzzlePos but does not have a line of fire with aimFromPos the unit will not shoot with this PR, but it will shoot in master.

My mental image is that the process is as follows:

  1. A preliminary check is done with TryTarget(..., preFire=false) to see if the target is viable in principle.

  2. The weapon calls AimWeapon and the unit starts rotating/moving arm etc... until the unit script reports angleGood = true.

  3. CanFire() checks angleGood, reload state, target availability, direction, etc. Then does the exact check with TryTarget(..., preFire=true) to see if there is a shot from the actual current muzzlePos.

  4. After resource and BlockShot checks, the weapon fires.

So the issue with rocket bots not shooting over walls ( beyond-all-reason/Beyond-All-Reason#9055 ) is that the preliminary check (1) fails because the actual muzzlePos is used and when the arm is down it is too low to shoot over walls. The bot doesn't even try to aim.

The issue #3241 is the other way around: The preliminary check determines that the unit can shoot which stops the movement of the unit, but then after aiming the TryTarget(..., preFire=true) no longer returns a viable shot, causing the unit to stop and do nothing when it could continue moving to a better fire position.

This PR modifies the preliminary check in 1) to use aimFromPos instead of muzzlePos. However that means if the actual muzzlePos IS able to shoot at the target while the aimFromPos is not, we degrade the result because the unit won't shoot where it would have shot before (even without rotating)

I see two hacky solutions to the "rocketbot shooting over walls" thingy

a) simply make two checks, check if a shot is possible from muzzlePos or aimFromPos so we shoot in all cases we shot before, but we now also allow aiming when a shot is possible with arm down.

The downside is that we now create more situations where the preliminary check is true and the bug in #3241 could happen more often.

b) make the check from muzzlePos if arm is raised to replicate existing behavior and use aimFromPos if arm is lowered. This would fix the issue at hand and not introduce new issues, but it seems like a very specific change for rocket bots and still ignores cases where rotation would allow shooting.

The perfect solution would be to adjust the preliminary check to check the muzzle position where it would be after aiming. But this is of course depending on the scripts. Add to that that there might units that can only aim when they stop moving.

Ideas?

@eun-ice

eun-ice commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

So instead of using a static weapon specific aimFromPos or the current muzzlePos we could also estimate where the weapon would be after aiming.
I do not know if this is viable, but here is an idea ho that could look like: #3327

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants