Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

adds Hovers WorldVisualOffset to muzzle calculations #21252

Merged
merged 1 commit into from Jan 6, 2024

Conversation

CDVoidwalker
Copy link
Contributor

This is a simple change that makes muzzle take in current world offset from hovers trait, if one is present. This way units such as hover MRLS have accurate offset positions.

OpenRA.Mods.Common/Traits/Armament.cs Outdated Show resolved Hide resolved
@@ -110,6 +111,8 @@ public class Armament : PausableConditionalTrait<ArmamentInfo>, ITick
public readonly WeaponInfo Weapon;
public readonly Barrel[] Barrels;
Turreted turret;
Hovers hovers;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it feels a bit weird to hardcode Hovers here, but I suppose it's fine

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it could be implemented via some interface IModifiesMuzzleOffset or something but since it's only trait that does that for now I suppose it's overkill.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't things like slopes and tilting from explosions also need to modify the offset?

Copy link
Member

@PunkPun PunkPun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's a bit weird how combat geometry lags behind a bit. or perhaps the visual is ahead of the curve?

hover

For some reason this PR is 523 commits behind. Please squash the commits

@abcdefg30
Copy link
Member

For some reason this PR is 523 commits behind

Rebasing with upstream/bleed should fix that. I'd advise rebasing the local bleed branch as well and then pushing that to your fork. Then new branches don't start out that much behind. (Or you create new branches from upstream/bleed in the future.)

@CDVoidwalker
Copy link
Contributor Author

For some reason this PR is 523 commits behind. Please squash the commits

Got used to working on non forked repo at work so I forgot to update my fork branches. Rebased on upstream bleed and squashed.

@abcdefg30 Slopes already modify the offsets, don't know about tilting from explosions.

@PunkPun PunkPun merged commit 680144b into OpenRA:bleed Jan 6, 2024
3 checks passed
@PunkPun
Copy link
Member

PunkPun commented Jan 6, 2024

Changelog

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

Successfully merging this pull request may close these issues.

None yet

3 participants