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.

Sign up for GitHub

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

Jump to bottom

Land mine armament #33883

Open
wants to merge 39 commits into
base: master
Choose a base branch
from
Open

Land mine armament #33883

wants to merge 39 commits into from
+203 −29

Conversation

kaiserbirch
Copy link

About the PR

The landmines are now armable and won't trigger unless armed.

Why / Balance

By making mines armable they are more safely handled and the amount of accidental explosions will decrease.

Technical details

Modified the component and system of landmine, added verb to ftl.

Media

rider64_alahZXmttQ.mp4

Requirements

Breaking changes

Changelog

  • tweak: Mines have to be armed before they can be triggered.

@github-actions github-actions bot added size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 16, 2024
@lzk228
Copy link
Contributor

lzk228 commented Dec 16, 2024

i think you should add an already armed version for mapping

beck-thompson
beck-thompson reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Really good first PR! And yeah, it would probably be a good idea to make an already armed version

@beck-thompson beck-thompson added P3: Standard Priority: Default priority for repository items. T: New Feature Type: New feature or content, or extending existing content D3: Low Difficulty: Some codebase knowledge required. A: Roundflow/Antag Area: Roundflow - "What happens in the game", including antagonist roles and their capabilities and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Dec 16, 2024
@Plykiya
Copy link
Contributor

Plykiya commented Dec 16, 2024 •
edited
Loading

the light shouldn't be blinking while it's not armed and it should also be in the examine text whether the landmine is armed or not

Franz - Josef Björck added 2 commits December 16, 2024 12:27
@kaiserbirch
Copy link
Author

i think you should add an already armed version for mapping

Is this solved by adding an armed version through YAML?

slarticodefast
slarticodefast requested changes Dec 16, 2024
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

A few more changes are needed:

  • Inherit SharedLandmineSystem on the client as well so the verb gets properly predicted. Currently if you right click on a mine you see it pop up after a short moment. Prediction means it will be there immediately.
  • The red blinking light on the land mine should only show when armed for visual feedback. Have a look at the GenericVisualizer for that.
  • Some maps have landmines mapped, for example in front of the bridge. You will have to edit the prototypes to make sure they still start armed.

@slarticodefast slarticodefast added the S: Awaiting Changes Status: Changes are required before another review can happen label Dec 16, 2024
@slarticodefast slarticodefast self-assigned this Dec 16, 2024
@github-actions github-actions bot added size/L Denotes a PR that changes 1000-4999 lines. and removed size/M Denotes a PR that changes 100-999 lines. labels Dec 16, 2024
lzk228
lzk228 suggested changes Dec 16, 2024
View reviewed changes
Copy link
Contributor

@lzk228 lzk228 left a comment

Choose a reason for hiding this comment

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

leave LandMineExplosive unchanged
and then just add

- type: entity
  parent: LandMineExplosive
  id: LandMineExplosiveArmed
  suffix: armed
  components:
  - type: LandMine
    armed: true

@github-actions github-actions bot removed the size/L Denotes a PR that changes 1000-4999 lines. label Dec 20, 2024
@kaiserbirch
Copy link
Author

Would a disarmament mechanic warrant its own PR or should I expand the scope of this one?

@kaiserbirch
Copy link
Author

@slarticodefast Are there any more changes that you request?

Aeshus
Aeshus previously requested changes Dec 22, 2024
View reviewed changes
Copy link
Contributor

@Aeshus Aeshus left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I have some thoughts about the way you handle the armable activation, as you should move some behavior between your TryArming and ArmingDone functions to be closer to the intended purpose of those events.

From my reading of the undocumented event code, ItemToggleActivateAttemptEvent is for checking/canceling the toggle if some conditions are not met, and ItemToggledEvent is for actually activating the functionality given it was not canceled.

The rest are all just minor stylistic nitpicks.

Thanks again.

@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Jan 10, 2025
@kaiserbirch kaiserbirch requested a review from Aeshus January 10, 2025 20:45
beck-thompson
beck-thompson requested changes Jan 19, 2025
View reviewed changes
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Looks good, just one minor style thing!

beck-thompson
beck-thompson approved these changes Jan 21, 2025
View reviewed changes
Copy link
Contributor

@beck-thompson beck-thompson left a comment

Choose a reason for hiding this comment

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

Looks good! Great first pr 🫡

@beck-thompson beck-thompson added the S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. label Jan 21, 2025
@Aeshus Aeshus dismissed their stale review January 30, 2025 15:21

Outdated review

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

@lzk228 lzk228 lzk228 requested changes

@beck-thompson beck-thompson beck-thompson approved these changes

@slarticodefast slarticodefast Awaiting requested review from slarticodefast

@Aeshus Aeshus Awaiting requested review from Aeshus

Requested changes must be addressed to merge this pull request.

Assignees

@slarticodefast slarticodefast

Labels
A: Roundflow/Antag Area: Roundflow - "What happens in the game", including antagonist roles and their capabilities D3: Low Difficulty: Some codebase knowledge required. P3: Standard Priority: Default priority for repository items. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kaiserbirch @lzk228 @Plykiya @slarticodefast @Aeshus @beck-thompson