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

Fix a bug where shields could occasionally be pierced #3448

Merged
merged 1 commit into from
May 28, 2021

Conversation

Baezon
Copy link
Member

@Baezon Baezon commented May 26, 2021

...despite full shields in that quadrant. If the ship moves towards the weapon in the frame, the next collision check for the weapon may start it inside the shield where last frame it may have ended just outside the shield. By adding in the ship's velocity we essentially take into account the apparent motion of the weapon from the ship's own moving reference frame. This was tested and works with regular and highly adversarial examples, such as very slow moving weapons which are normally very difficult to actually trigger collisions against for this reason.

The bigger issue, however, is backwards compatibility. I think almost everyone has noticed this happen at some point or another, and although I haven't specifically tested for it, I highly suspect this is the reason Seraphim and Nephilim bombers can occasionally be destroyed in a single volley of trebuchets, despite the stats on the paper not allowing for this. This could be behind a flag (possibly enabled by $Target version) but I am loathe to do this because of how simple, clearly a bug, and fundamental this is to the collision system.

@Baezon Baezon added fix A fix for bugs, not-a-bugs, and/or regressions. discussion This issue has (or wants) a discussion physics A feature or issue related to the physics algorithms labels May 26, 2021
@JohnAFernandez
Copy link
Contributor

It is a definite bug. It's just a balance affecting bug... So even though it was never intended, it may be better to put it behind a $Target Version, as loathsome as I agree it is.

@Goober5000
Copy link
Contributor

I concur. We have put less obvious bugs behind flags.

I would add it to ai_profiles and then set the flag to be automatically activated for 21.4+, similar to several other ai_profiles flags.

@Baezon
Copy link
Member Author

Baezon commented May 26, 2021

I figured as much, although I'd prefer to avoid putting this in ai_profiles since it is not AI related, in lieu of something like game_settings similar to $Fixed Turret Collisions

@Goober5000
Copy link
Contributor

AI_Profiles probably should have had a different name from the beginning. But the main difference between ai_profiles and game_settings is that ai_profiles can be changed on a per-mission basis, while game_settings applies mod-wide. Therefore, a lot of flags affecting balance have been added to ai_profiles. This allows missions created before the flag to use the old setting, while missions created after the flag can use the new setting. (Scroll uses this a lot because of its long development history.)

@Baezon
Copy link
Member Author

Baezon commented May 27, 2021

Fair enough, I thought 'fixed turret collisions' was under target version, but apparently not. And ai_profiles already other not really AI-related stuff anyway.

fixed ship-weapon collisions isn't really a good name, but I wasn't sure what else to call it (this effect is not specific to shields).

Copy link
Contributor

@JohnAFernandez JohnAFernandez left a comment

Choose a reason for hiding this comment

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

I agree that this is the best name that I can think of, too.

Copy link
Contributor

@Goober5000 Goober5000 left a comment

Choose a reason for hiding this comment

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

LGTM too.

@JohnAFernandez JohnAFernandez merged commit edcdbd0 into scp-fs2open:master May 28, 2021
@JohnAFernandez JohnAFernandez added the Not Eligible for Point Release An already merged bugfix that adjusts balance or changes expected behavior and cannot be a backfix label Jun 14, 2021
@JohnAFernandez JohnAFernandez removed the Not Eligible for Point Release An already merged bugfix that adjusts balance or changes expected behavior and cannot be a backfix label Jun 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion This issue has (or wants) a discussion fix A fix for bugs, not-a-bugs, and/or regressions. physics A feature or issue related to the physics algorithms
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants