-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 firework rocket & firework star #5455
base: minor-next
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks mostly OK. I haven't tested it yet.
My main sore point on this PR is the woefully inadequate documentation and ambiguous method names.
Missing features:
|
Also with crossbows, but it can wait until those are implemented first. |
A PR doesn't have to be all in one, I think the current features are sufficient, the remaining ones are not critical. |
Co-authored-by: ipad54 <[email protected]>
For posterity: firework rocket entity also has a long metadata property We don't have elytras implemented right now so I think we don't have to bother about this and the PR looks finished for me. |
|
||
/** | ||
* TODO: The entity should be saved and loaded, but this is not possible. | ||
* @see https://bugs.mojang.com/browse/MCPE-165230 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not liking this. We should still save & restore them in PM, regardless of vanilla bugs that don't store them.
/** | ||
* Returns maximum number of ticks this will live for. | ||
*/ | ||
public function getLifeTicks() : int{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getMaxAgeTicks()
would be more clear IMO
* | ||
* @return $this | ||
*/ | ||
public function setLifeTicks(int $lifeTicks) : self{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto here
$position = match($face){ | ||
Facing::DOWN => $position->add(0, -$correction, 0), | ||
Facing::UP => $position->add(0, $correction, 0), | ||
Facing::NORTH => $position->add(0, 0, -$correction), | ||
Facing::SOUTH => $position->add(0, 0, $correction), | ||
Facing::WEST => $position->add(-$correction, 0, 0), | ||
Facing::EAST => $position->add($correction, 0, 0), | ||
default => throw new AssumptionFailedError("Invalid facing $face") | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$position = match($face){ | |
Facing::DOWN => $position->add(0, -$correction, 0), | |
Facing::UP => $position->add(0, $correction, 0), | |
Facing::NORTH => $position->add(0, 0, -$correction), | |
Facing::SOUTH => $position->add(0, 0, $correction), | |
Facing::WEST => $position->add(-$correction, 0, 0), | |
Facing::EAST => $position->add($correction, 0, 0), | |
default => throw new AssumptionFailedError("Invalid facing $face") | |
}; | |
$position = $position->add(Vector3::zero()->getSide($face)->multiply($correction)); |
If we allowed getSide()
to accept float values, this would be even simpler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to pmmp/Math#86
default => throw new AssumptionFailedError("Invalid facing $face") | ||
}; | ||
|
||
$randomDuration = (($this->flightDurationMultiplier + 1) * 10) + mt_rand(0, 12); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer if these magic numbers were moved to constants.
Introduction
Implement items:
Firework Rocket
,Firework Star
and entity:Firework Rocket
.Follow-up
Requires translations:
death.attack.fireworks
{%0} went off with a bang
Tests
https://youtu.be/kHs_nBmLRhs