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

Feature: Broodmother alerts/messages and damage indicator support #2325

Merged
merged 4 commits into from
Sep 12, 2024

Conversation

MTOnline69
Copy link
Contributor

@MTOnline69 MTOnline69 commented Aug 11, 2024

What

Adds Broodmother to the damage indicator, and a new category to the Combat features for various Broodmother alerts, messages and a countdown:

  • Shows a countdown on screen once we can figure out when the Broodmother will spawn (must observe a stage change)
  • Warns the player when there is 1 minute left until the Broodmother will spawn
  • Alerts the player with a title and sound that the Broodmother spawned, with a link in chat to quickly warp
    • Alert sound, pitch, amount of times repeated and title text can all be changed
  • Sends chat messages upon each Broodmother spawn stage change (choose which stages to get notified about)
Images

image
image
image

Since i'm still new to contributing, my code may need work.

Changelog New Features

  • Added Broodmother spawn alert, countdown and stage change messages. - MTOnline
    • Countdown will not show until a spawning stage change is observed, and may be off by a few seconds.
  • Added Broodmother support to the Damage Indicator. - MTOnline

@hannibal002 hannibal002 added this to the Version 0.27 milestone Aug 11, 2024
@jani270 jani270 added the Soon This Pull Request will be merged within the next couple of betas label Aug 26, 2024
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

The features work fine for me in the game.
The code looks great. You use a ton of different features of the SkyHanni framework perfectly fine.

I've found a couple of suggestions that could improve the maintainability and consistency of the code, nothing big, though. Thanks for the cool PR!

@qtlunya
Copy link
Contributor

qtlunya commented Sep 10, 2024

One thing, the Spider Mound technically encompasses the whole area, including at the bottom of the mountain which is the early game spider area. It may be better to refer to it as the "Top of the Nest", as the travel scroll and the warp menu do in the game, and the check for whether the player is in the Spider Mound should probably reuse getBroodmotherShowWhen from the ScoreboardEvent class instead (currently private, probably best to extract it into a utility function).

@MTOnline69
Copy link
Contributor Author

One thing, the Spider Mound technically encompasses the whole area, including at the bottom of the mountain which is the early game spider area. It may be better to refer to it as the "Top of the Nest", as the travel scroll and the warp menu do in the game, and the check for whether the player is in the Spider Mound should probably reuse getBroodmotherShowWhen from the ScoreboardEvent class instead (currently private, probably best to extract it into a utility function).

Alright, I'll change the text. Which utils class would getBroodmotherShowWhen best fit?

@qtlunya
Copy link
Contributor

qtlunya commented Sep 10, 2024

LocationUtils is probably a good choice, could rename the method to isAtTopOfNest or similar.

@hannibal002
Copy link
Owner

LocationUtils is probably a good choice, could rename the method to isAtTopOfNest or similar.

I would like to have the abstract util classes as abstract as possible. Sure, this is location related. But this check is only relevant for spiders den features, for nothing else that touches location. Therefore a newly created SpidersDenAPI class feels better imo

@github-actions github-actions bot added the Merge Conflicts There are open merge conflicts with the beta branch. label Sep 11, 2024
Copy link

This pull request has conflicts with the base branch "beta". Please resolve those so we can test out your changes.

@MTOnline69 MTOnline69 closed this Sep 11, 2024
@github-actions github-actions bot removed Merge Conflicts There are open merge conflicts with the beta branch. Soon This Pull Request will be merged within the next couple of betas labels Sep 11, 2024
@MTOnline69 MTOnline69 reopened this Sep 11, 2024
@MTOnline69
Copy link
Contributor Author

Ignore the absolute mess I made of this PR earlier

All requested changes dealt with and I tested quite extensively with no issues.

@hannibal002 hannibal002 added the Soon This Pull Request will be merged within the next couple of betas label Sep 12, 2024
Copy link
Owner

@hannibal002 hannibal002 left a comment

Choose a reason for hiding this comment

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

LGTM

@hannibal002 hannibal002 merged commit 4e23db7 into hannibal002:beta Sep 12, 2024
5 checks passed
@github-actions github-actions bot removed the Soon This Pull Request will be merged within the next couple of betas label Sep 12, 2024
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.

4 participants