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

[Slayers] Adds Slayer Bossbars #940

Merged
merged 2 commits into from
Sep 8, 2024

Conversation

BigloBot
Copy link
Contributor

@BigloBot BigloBot commented Aug 19, 2024

Slayer Bossbars innit

@LifeIsAParadox LifeIsAParadox added the wip This PR is a work in progress label Aug 19, 2024
@BigloBot BigloBot changed the title [Slayers] Adds Bossbars [Slayers] Adds Slayer Bossbars Aug 19, 2024
@AzureAaron AzureAaron added the new feature This issue or PR is a new feature label Aug 19, 2024
@AzureAaron AzureAaron added this to the 1.23.0 milestone Aug 19, 2024
@AzureAaron
Copy link
Collaborator

Should cover #908

@AzureAaron AzureAaron added the merge conflicts This PR has merge conflicts that need solving. label Sep 4, 2024
@LifeIsAParadox LifeIsAParadox removed the merge conflicts This PR has merge conflicts that need solving. label Sep 7, 2024
@BigloBot BigloBot marked this pull request as ready for review September 7, 2024 22:56
@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed wip This PR is a work in progress labels Sep 7, 2024
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.

I don't think the render mixin is necessary. I would add it to BossBarHud#bossBars and remove it when you set the bar to null. The current implementation also stops any other bars from rendering.

@LifeIsAParadox LifeIsAParadox added changes requested This PR need changes and removed reviews needed This PR needs reviews labels Sep 8, 2024
@BigloBot
Copy link
Contributor Author

BigloBot commented Sep 8, 2024

I don't think the render mixin is necessary. I would add it to BossBarHud#bossBars and remove it when you set the bar to null. The current implementation also stops any other bars from rendering.

I mixin'd on render to be able to explictly cancel other bars from rendering. I originally had it to just appened a bar but people disliked multiple bars in the video i demo'd it in, thus i mixin'd to render to cancel any 'quest' bars for the duration of the fight

@kevinthegreat1
Copy link
Collaborator

Ok, I'll just make some naming and formatting fixes then.

@LifeIsAParadox LifeIsAParadox added reviews needed This PR needs reviews and removed changes requested This PR need changes labels Sep 8, 2024
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.

Works well, thanks!

Note

Squash please

@LifeIsAParadox LifeIsAParadox added merge me please Pull requests that are ready to merge and removed reviews needed This PR needs reviews labels Sep 8, 2024
@viciscat viciscat merged commit d241b8c into SkyblockerMod:master Sep 8, 2024
1 check passed
@LifeIsAParadox LifeIsAParadox removed the merge me please Pull requests that are ready to merge label Sep 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature This issue or PR is a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants