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

Slayer System rework #1040

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

Slayer System rework #1040

wants to merge 11 commits into from

Conversation

7azeemm
Copy link
Contributor

@7azeemm 7azeemm commented Nov 1, 2024

  • Added Slayer HUD
  • Added Boss slain time
  • Added Personal Best slain time
  • Added Boss and MiniBoss spawn alert
  • Added Mute Enderman sounds
  • Added Lazer phase Timer

@7azeemm
Added Slayer HUD
8d04e75 
Added Boss slain time
Added Personal Best slain time
Added Boss and MiniBoss spawn alert
Added Mute Enderman sounds
Added Lazer phase Timer
@LifeIsAParadox LifeIsAParadox added the reviews needed This PR needs reviews label Nov 1, 2024
@7azeemm
Copy link
Contributor Author

7azeemm commented Nov 1, 2024

image
just need help fixing this
i didn't figure out how to make text renders behind entity
also glow effect is broken (filled not outline like in img) in beta 1 for only 1.21.3 minecraft version, beta 1 1.21.1 works fine. (same problem without sodium)

idk if that's a good color combo in hud
i'm bad at designing
image

@Emirlol
Copy link
Collaborator

Emirlol commented Nov 1, 2024

Looks pretty good.
I went ahead and fixed the filled outline issue, along with a few other things that kept crashing my game so that I could actually test it. Hope you don't mind.

@7azeemm
Copy link
Contributor Author

7azeemm commented Nov 1, 2024

nice, appreciate your help

Emirlol
Emirlol requested changes Nov 1, 2024
View reviewed changes
Copy link
Collaborator

@Emirlol Emirlol 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 nitpicks here and there but the code looks good in general. I didn't look too deep into the logic since stuff just seemed to work, excluding the LazerTimer as I wasn't able to test it. (too weak for voidgloom seraph lol)

src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java

@Init
public static void init() {
actions.put(SlayerAction.CANCELLED, () -> quest = null);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This actions system could be converted to use actual methods and call those methods depending on the actual string value of the matched group from the regex.

While this might be a shorter way to do the same thing, it's not quite obvious from first look what it is for, and adds unnecessary complexity.

src/main/java/de/hysky/skyblocker/skyblock/slayers/hud/SlayerHudWidget.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java
Comment on lines +148 to +160
public static int calculateBossesNeeded() {
int tier = RomanNumerals.romanToDecimal(slayerTier);
if (tier == 0) return -1;

int xpPerTier;
if (slayerType.equals("Vampire")) {
xpPerTier = SlayerConstants.vampireXpPerTier[tier - 1];
} else {
xpPerTier = SlayerConstants.regularXpPerTier[tier - 1];
}

return (int) Math.ceil((double) xpRemaining / xpPerTier);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This method doesn't account for the xp bonus of Aatrox mayor/minister.

src/main/resources/assets/skyblocker/lang/en_us.json Outdated Show resolved Hide resolved
src/main/resources/assets/skyblocker/lang/en_us.json Outdated Show resolved Hide resolved
src/main/resources/skyblocker.mixins.json Outdated Show resolved Hide resolved
@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Nov 1, 2024
@Emirlol Emirlol added the new feature This issue or PR is a new feature label Nov 1, 2024
BigloBot
BigloBot reviewed Nov 4, 2024
View reviewed changes
src/main/java/de/hysky/skyblocker/skyblock/slayers/SlayerManager.java
private static final Logger LOGGER = LoggerFactory.getLogger(SlayerManager.class);
private static final Map<SlayerAction, Runnable> actions = new HashMap<>();
private static final Pattern SLAYER_PATTERN = Pattern.compile("Revenant Horror|Tarantula Broodfather|Sven Packmaster|Voidgloom Seraph|Inferno Demonlord|Riftstalker Bloodfiend");
private static final Pattern SLAYER_TIER_PATTERN = Pattern.compile("^(Revenant Horror|Tarantula Broodfather|Sven Packmaster|Voidgloom Seraph|Inferno Demonlord|Riftstalker Bloodfiend)\\s+(I|II|III|IV|V)$");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hypixel decided to make the Riftstalker Bloodfiend break the pattern. It's in game nametag does not contain its full name. I know this is an existing bug but it'd be great if u could fix it.

kevinthegreat1
kevinthegreat1 reviewed Nov 4, 2024
View reviewed changes
Copy link
Collaborator

@kevinthegreat1 kevinthegreat1 left a comment

Choose a reason for hiding this comment

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

Not a complete review yet, but just some comments.

src/main/java/de/hysky/skyblocker/mixins/SoundSystemMixin.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/mixins/WorldRendererMixin.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/mixins/WorldRendererMixin.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/mixins/WorldRendererMixin.java Outdated Show resolved Hide resolved
src/main/java/de/hysky/skyblocker/utils/Utils.java Outdated Show resolved Hide resolved
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Nov 4, 2024
7azeemm and others added 5 commits November 4, 2024 21:08
@7azeemm
Translation keys sorting
3e616a2
@7azeemm @kevinthegreat1
mixins
f8c6aba 
Co-authored-by: Kevin <92656833+kevinthegreat1@users.noreply.github.com>
@7azeemm @kevinthegreat1
more mixins changes
a675a49 
Co-authored-by: Kevin <92656833+kevinthegreat1@users.noreply.github.com>
@7azeemm @kevinthegreat1
..
d8ca611 
Co-authored-by: Kevin <92656833+kevinthegreat1@users.noreply.github.com>
@7azeemm
more translatable text
4f4a15b 
mixins changes
shows now previous personalBest
and more
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@BigloBot BigloBot BigloBot left review comments

@kevinthegreat1 kevinthegreat1 kevinthegreat1 left review comments

@Emirlol Emirlol Emirlol requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
new feature This issue or PR is a new feature reviews needed This PR needs reviews
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@7azeemm @Emirlol @kevinthegreat1 @BigloBot @LifeIsAParadox