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

Basic Looting Enchantment implementation #6

Merged
merged 10 commits into from Jan 28, 2020

Conversation

iZeaoGamer
Copy link
Contributor

@iZeaoGamer iZeaoGamer commented Jan 28, 2020

Okay, I saw some of the code had some basic Looting enchantment implementation. It was okay, but didn't fit its needs of a looting basic enchantment. I did see this on your to-do list though, so I thought I'd save you the bother of doing it yourself, and allowing me to implement this. By "Basic Looting Enchantment", I'm assuming you mean add Looting enchantment drops on all of the mobs?
If so, then this is the Pull Request for you.

What does this Pull Request add to the plugin?

So this plugin adds basic looting enchantment on all of the mobs that include getDrops() array function. Those that do not, were probably missed out on by the plugin on purpose. If not, I'll just let you implement more drops for the other mobs. If not, then don't worry. I never conflicted those with the non dropped mobs.
This plugin also implements rewrites to the Looting enchantment. This is because adding a + before the looting level isn't very efficient, especially in vanilla. This needs to multiply by the looting level, not adding onto it.
This isn't how vanilla works.

So for example, if I had a looting 3 enchantment level, it'd multiply the drops by 3. Adding onto it just doesn't work very well with Looting. Since Looting levels matter. If you add onto the looting levels, then looting levels are basically pointless, because it's adding one onto it every by level, and not multiplying it. This is why I made a recode with Looting levels. No, this shouldn't require any major plugins for this to work.

Along with this, I've also ticked off Basic looting enchantment implementation in the Readme.MD file just so we know where we are at.

Has this Pull Request been tested?

Start up script - Tested to enable with success.
Mob killing / kills - Untested, but should work. There shouldn't be any other major issues to worry about. Untested, but everything should work as expected.
Thank you!

Let's recape! What have we fixed in this Pull Request?

We have made the following changes:

  • Added Zombies to the Lootable mobs. Before, this required another plugin for this to run.
  • Fixed errors when killing Snow golems.
  • Added use import for EntityDamageByEntityEvent for Snow golems.
  • PHP Stan Level 8 has successfully passed. (:
  • Fixed random server crashes when sometimes killing mobs. This was due to $looting = 1l not being defined at the beginnging of the getDrops() array code. - Thanks @Heisenburger69 for pointing that out!
  • Re-coded Looting enchantment - To allow multiplications, rather than adding looting levels onto the mob drops. All fixed now.
  • I kept adding, then removing Villagers. In this update, there's no need to add Villagers because of two reasons.
  1. Villagers are already implemented into Pocketmine-MP.
  2. Villagers by default don't drop anything, according to Pocketmine's code.

Yes, zombies are already implemented into Pocketmine too, but there are drops, which means it should count as a lootable mob. When I mean Lootable, I mean those that drop stuff when killed.

iZeaoGamer added 3 commits January 28, 2020 15:39
This includes all mob drops in this plugin - If a player kills a mob using their looting sword, the drops will multiply depending on their looting level. Example: If a player has a looting 3, the drops will multiply by 3.
Whoops, was adding this for testing reasons. Wasn't meant to be added to this plugin.
@varunbln
Copy link
Owner

The PR looks good except some things need to be changed up + bug fixes
EntityDamageByEntityEvent has not been imported in SnowGolem.php
Further,
image
should be replaced by defining $lootingL = 1, then setting it to $looting->getLevel() inside the if. You can look at the PHPStan report for more details

@varunbln
Copy link
Owner

varunbln commented Jan 28, 2020

I meant $lootingL = 1; if($looting !== null) { $lootingL = $looting->getLevel(); }

@iZeaoGamer iZeaoGamer closed this Jan 28, 2020
@iZeaoGamer iZeaoGamer reopened this Jan 28, 2020
@iZeaoGamer
Copy link
Contributor Author

@Heisenburger69 Everything's fixed. Sorry about closing the issue. Was an accident. Please merge if you can. Thank you!

@varunbln
Copy link
Owner

Looks great, I'll merge now. Thank you!

@varunbln varunbln merged commit c8ca61d into varunbln:master Jan 28, 2020
@iZeaoGamer
Copy link
Contributor Author

Okay, thank you.

@AGTHARN AGTHARN mentioned this pull request Apr 18, 2020
@AGTHARN AGTHARN mentioned this pull request May 3, 2020
@Hellohi3654 Hellohi3654 mentioned this pull request May 19, 2020
This was referenced Jun 1, 2020
@MonoAdrian23 MonoAdrian23 mentioned this pull request Jul 29, 2020
@abcdavk abcdavk mentioned this pull request Aug 18, 2021
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.

None yet

2 participants