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

Implement Goat horns #5232

Merged
merged 21 commits into from
Nov 14, 2024
Merged

Implement Goat horns #5232

merged 21 commits into from
Nov 14, 2024

Conversation

IvanCraft623
Copy link
Member

@IvanCraft623 IvanCraft623 commented Aug 17, 2022

Introduction

Implement Goat horns and its sounds

Follow-up

Tests

Goat.horns.test.mp4

@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
@IvanCraft623 IvanCraft623 marked this pull request as ready for review December 23, 2022 22:07
@IvanCraft623 IvanCraft623 changed the base branch from major-next to minor-next June 4, 2023 22:31
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

The PR looks mostly fine, but I'm dubious about the way the sounds are handled.

I feel like the sounds could be done with much less code by having a single GoatHornSound class which accepts GoatHornType and match()es the sound ID.

Is there some advantage to having a class per sound that I'm not seeing?

src/item/GoatHornType.php Outdated Show resolved Hide resolved
dktapps
dktapps previously requested changes Jun 5, 2023
src/item/GoatHorn.php Outdated Show resolved Hide resolved
src/item/GoatHorn.php Outdated Show resolved Hide resolved
- Usage of new native enums instead of EnumTrait
- Usage of new IntSaveIdMapTrait
- Remove unnecesary GoatHorn*Sound classes
src/item/GoatHornType.php Outdated Show resolved Hide resolved
Copy link
Member

@ShockedPlot7560 ShockedPlot7560 left a comment

Choose a reason for hiding this comment

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

PR looks fine

@GamerMJay
Copy link

cooldown is missing :0

Joshy3282 added a commit to Joshy3282/PocketMine-MP that referenced this pull request Oct 2, 2024
@dktapps
Copy link
Member

dktapps commented Nov 9, 2024

This looks OK, although the cooldown is indeed missing. That should be easily addressed with the merge of #6405.

@ipad54
Copy link
Member

ipad54 commented Nov 11, 2024

I've added the missing cooldown.

Screenshot_20241111_134311

@dktapps dktapps requested a review from a team as a code owner November 14, 2024 13:54
Copy link
Member

@dktapps dktapps left a comment

Choose a reason for hiding this comment

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

Looks like this is ready to go

@dktapps dktapps merged commit 9b58d35 into pmmp:minor-next Nov 14, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Gameplay Related to Minecraft gameplay experience Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants