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

Add ExperienceOrb attracts lock feature #4623

Merged
merged 7 commits into from
Dec 15, 2021

Conversation

ShockedPlot7560
Copy link
Member

Introduction

As explained in the issue #4589 , there is a need to allow developers to remove players from the attraction of xp orbs.

Relevant issues

Changes

API changes

The following methods have been added to the ExperienceManager :

  • isAttractsLocked : Returns true if the Human no longer accepts xp orbs attracts, false otherwise
  • setAttractsLocked : The setter to change the acceptance status of xp orbs

Behavioural changes

In ExperienceOrb:

  • The getTargetPlayer method will no longer return an instance of Human if isAttractsLocked return true for the instance
  • The setTargetPlayer method will no longer choose a Human that returns true to isAttractsLocked
  • In entityBaseTick the orb will no longer choose a target that returns true to isAttractsLocked

Follow-up

No specific action is required

Tests

  • To test the normal system, mine a block of diamond ore in survival and the orb will indeed follow you.
  • To test the locked system, create a plugin that disables orbs by default for any given player and it will look like this:
    image

@ShockedPlot7560 ShockedPlot7560 changed the title Add ExperienceOrb attracts lock feature, fixed pmmp/pocketmine-mp#4589 Add ExperienceOrb attracts lock feature Dec 7, 2021
@ColinHDev
Copy link
Contributor

Unexpected, but an interesting-looking solution.

$this->targetPlayerRuntimeId = $player !== null ? $player->getId() : null;
}else{
$this->targetPlayerRuntimeId = null;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks really overcomplicated. This would be much simpler

if($player !== null && !$player->getXpManager()->isAttractsLocked()){
			$this->targetPlayerRuntimeId = $player->getId();
		}else{
			$this->targetPlayerRuntimeId = null;
		}

or

$this->targetPlayerRuntimeId = $player !== null && !$player->getXpManager()->isAttractsLocked() ? $player->getId() : null;

But does this check even belong there? In every case, where pm would set the target player itself, it does the checks alone. So shouldn't plugin devs be required to do their own checks? Possibly someone wants to bypass the lock of another plugin (whyever).

Copy link
Member

Choose a reason for hiding this comment

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

No, it doesn't belong here. If the player can't be tracked, the orb should try to find a different target.

Copy link
Member

Choose a reason for hiding this comment

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

This whole change is redundant.

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.

Code looks OK (as in it will work) but attractsLocked is a really abysmal name.

@ShockedPlot7560
Copy link
Member Author

Wouldn't it be better to call it :

  • attractsExperienceOrb
    or
  • orbSeverity

It must be made clear in the name that this property must be able to be used to cut the gravitation of the orbs around the player and remove his ability to recover them

I otherwise agree that the changes in setTargetPlayer or getTargetPlayer its rather redundant and deserves to be removed to allow the developers more freedom.

* removal of conditions in getTargetPlayer and setTargetPlayer
* entityBaseTick only will check the condition
@dktapps
Copy link
Member

dktapps commented Dec 8, 2021

canAttractXpOrbs and setCanAttractXpOrbs would be better (for consistency with other methods).

@ShockedPlot7560
Copy link
Member Author

Indeed, it makes a little more sense

@dktapps dktapps changed the base branch from stable to next-minor December 15, 2021 04:28
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.

Tested and found working.

@dktapps dktapps added Category: API Related to the plugin API Type: Contribution labels Dec 15, 2021
@dktapps dktapps added this to the 4.1 milestone Dec 15, 2021
@dktapps dktapps merged commit de82424 into pmmp:next-minor Dec 15, 2021
@dktapps dktapps added Type: Enhancement Contributes features or other improvements to PocketMine-MP and removed Type: Contribution labels Nov 26, 2022
@ShockedPlot7560 ShockedPlot7560 deleted the ExperienceOrb branch June 4, 2023 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: API Related to the plugin API Type: Enhancement Contributes features or other improvements to PocketMine-MP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ExperienceOrb: no possibility of interfering in search for target entity
3 participants