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

Added breeding support #5116

Closed
wants to merge 0 commits into from

Conversation

Fusezion
Copy link
Contributor

@Fusezion Fusezion commented Sep 24, 2022

Description

This PR adds the basis of breeding into skript

# Event
[on] [entity] breed[ing]

# Expressions
bred child
breeding mother
breeding father

image
ignore the <none> was making sure event-entity = child
event-entity acts the same as bred child
event-livingentity returns the entity who started the breeding
event-item returns the itemstack used for breeding

Note

as for the experience changes I've copied the ones from the PR #4284


Target Minecraft Versions: 1.12.2+ (Skript 2.7 will remove this requirement)
Requirements: MC 1.12.2
Related Issues: #5111

@Fusezion Fusezion changed the title Adding breeding support Added breeding support Sep 24, 2022
@Fusezion
Copy link
Contributor Author

Fusezion commented Sep 24, 2022

I'll see about adding the stuff under breedable later

@TheLimeGlass
Copy link
Collaborator

There is already a pull request standardizing all the the event values to be enum.

@TheLimeGlass TheLimeGlass added the feature Pull request adding a new feature. label Sep 24, 2022
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Nice feature :D

I think ExprBredChild, ExprBreedingFather, and ExprBreedingMother should be combined into one expression (ex: ExprBreedingFamily) (idk a good name lol)

Once that expression is made, I'll do further review - please feel free to re-request my review

@@ -727,13 +728,13 @@ public Entity get(final PlayerInteractEntityEvent e) {
@Override
@Nullable
public ItemStack get(final PlayerInteractEntityEvent e) {
EquipmentSlot hand = e.getHand();
Copy link
Member

Choose a reason for hiding this comment

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

If possible, can you revert these unrelated changes?

@TheLimeGlass TheLimeGlass self-requested a review December 16, 2022 07:11
Copy link
Collaborator

@TheLimeGlass TheLimeGlass left a comment

Choose a reason for hiding this comment

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

Order of methods is init -> get/getAll -> acceptChange -> change -> setTime -> getTime -> isSingle -> getReturnType -> toString for SimpleExpression

The usage of event-father and event-mother can be used after Pickle's pull request at #4963 to distinguish between the many entities that can be involved in this event as there are many conflicts.

Comment on lines 893 to 958
@Nullable
@Override
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@Nullable
@Override
@Override
@Nullable

While here

Comment on lines 71 to 72
@Override
protected @Nullable LivingEntity[] get(Event event) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
@Override
protected @Nullable LivingEntity[] get(Event event) {
@Override
@Nullable
protected LivingEntity[] get(Event event) {

Same in other classes

public class ExprBredChild extends SimpleExpression<LivingEntity> {

static {
Skript.registerExpression(ExprBredChild.class, LivingEntity.class, ExpressionType.SIMPLE, "bred child");
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like [bred] child as a syntax


@Override
protected @Nullable LivingEntity[] get(Event event) {
return new LivingEntity[]{((EntityBreedEvent) event).getEntity()};
Copy link
Collaborator

Choose a reason for hiding this comment

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

When it's an event specific syntax, you should add an instanceof check to ensure it's the correct event.
Same in other classes

((ExperienceSpawnEvent) e).setSpawnedXP((int) d);
experience = 0;

experience = Math.max(0, Math.round(experience));
Copy link
Collaborator

Choose a reason for hiding this comment

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

You can't continue to have this round method. The round method originally existed for the double, which is now an integer from the looks of it. Math.round(integer) doesn't exist, it only does double and floats, so no clue how this is valid code unless it's somehow converting it to a double/float.

@TheLimeGlass TheLimeGlass added the 2.8 Targeting a 2.8.X version release label Jan 3, 2023
@Fusezion Fusezion closed this Apr 5, 2023
@Fusezion
Copy link
Contributor Author

Fusezion commented Apr 5, 2023

Whoops , oh well, I'll quickly fix some stuff up and reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.8 Targeting a 2.8.X version release feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants