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

Breedable & Ageable Stuff #5584

Closed
wants to merge 36 commits into from

Conversation

Fusezion
Copy link
Contributor

@Fusezion Fusezion commented Apr 7, 2023

Description

This PR aims to add breeding support along with syntax related to breeding and aging.

Added ✨ EntityBreedEvent MC 1.13+
Added ✨ EntityEnterLoveModeEvent PAPER 1.16+
Added ✨ ExprBreedingFamily MC 1.13+
Added ✨ CondCanAge, CondCanBreed MC 1.16+
Added ✨ EffLockAge, EffBreedable MC 1.16+
Added ✨ ExprLoveTime MC 1.16+ *if anyone has a better name please help
Updated 📋 ExprExperience

Added ✨ CondIsAdult MC 1.13+ (Req: 1.16+ for any non animal)
Added ✨ EffMakeAdult MC 1.13+ (Req: 1.16+ for any non animal)
*the method I used for telling if using 1.16+ if there's any other alternative please let me know

Breaks 🪦 Removes remove_all support from ExprExperience, people using it before will break

Some of these 1.16 events could of been added under the Ageable class instead of Breedable which would allow earlier versions, however I find it better to use Breedable in this case then Ageable as it makes more sense. If anyone would prefer Ageable please let me know.


Target Minecraft Versions: 1.13+, 1.16+ & Paper 1.16+
Requirements: Breed event 1.13, Enter love mode paper 1.16, rest 1.16
Related Issues: #5111
Additional: this PR is the updated one for #5116 I accidently did some issues as such just decided to recode it.
Majority if not all of the requested changes there should transfer here, however I've added much more syntax compared to before.

📎 Added EntityEnterLoveModeEvent
🐷 Added EntityBreedEvent
✨ Added EventValues for these events
👪 Added family expression for Father, Mother, Child, and Breeder
✨ Added breeding experience to experience class
*update class to current standards
✨ Added condition for checking ageable
✨ Added condition for checking breedable
✨ Added effect for toggle breedable
✨ Added effect for locking age
✨ Added expression for love ticks
🐛 Fixed issue with `livingentities`
🐛 Added class exist checks for Breedable instances
✨ Added RequirePlugins annotation to select classes
📋 Added test for ExprLoveTime & CondIsInLove
📋 Addes test for EffBreedable & CondCanBreed
📋 Added test fpr EffLockAge & CondCanAge
@Fusezion

This comment was marked as resolved.

✨ Added Condition for checking is adult
✨ Added Effect to make an entity a baby or adult
📋 Added test for these syntaxes
🔵 1.16 for any mob, below that is animals only
@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Apr 7, 2023
Copy link
Member

@AyhamAl-Ali AyhamAl-Ali left a comment

Choose a reason for hiding this comment

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

Nice PR ⚡, It's better to split features in multiple PRs next time to avoid such big review and requested changes :)

Thanks for all these formatting changes it's just what I wanted when waking up

Co-authored-by: Ayham Al Ali <[email protected]>
Fusezion and others added 5 commits April 7, 2023 21:45
✨ Renamed expression to ExprLoveTicks
✨ Added support for ticks via integers
📋 Added a new test for using integers
📋 Removed unneeded test condition
✨ Changed expression for father, mother and offspring a bit
✨ Converted get to a switch statement
🐛 Fixed an issue where set allowed more than one value
🐛 Fixed a typo in examples
✨ Added a spacer to examples
@Fusezion
Copy link
Contributor Author

Fusezion commented Apr 8, 2023

Failing on CondIsWithin.sk "failed within chunk # 1" 1.19.4 all other ones pass

@sovdeeth
Copy link
Member

sovdeeth commented Apr 8, 2023

Failing on CondIsWithin.sk "failed within chunk # 1" 1.19.4 all other ones pass

Fixed in #5587, not related to this PR.

@Fusezion Fusezion changed the title Breedable Stuff Breedable & Ageable Stuff Apr 8, 2023
@Fusezion
Copy link
Contributor Author

Fusezion commented Apr 8, 2023

Nice PR ⚡, It's better to split features in multiple PRs next time to avoid such big review and requested changes :)

Didn't intend to really add so much stuff, love ticks made sense due to breedable, and ageable was just close enough to each other I found it fair. Most requested changes seem to be formatting which is just caused by how little I mess with PRs it'll get better over time.

@AyhamAl-Ali
Copy link
Member

Didn't intend to really add so much stuff, love ticks made sense due to breedable, and ageable was just close enough to each other I found it fair. Most requested changes seem to be formatting which is just caused by how little I mess with PRs it'll get better over time.

All good for now. It's just easier for us to review and easier for you to handle requested changes as the larger the PR is the more time it requires to review/change/merge

As for the formatting stuff, it's just a matter of time as you stated.

@AyhamAl-Ali AyhamAl-Ali added the breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) label Apr 8, 2023
Just adds some notes in descriptions to bring more light to animal exclusive syntax

Co-Authored-By: Ayham Al Ali <[email protected]>
@Moderocky Moderocky changed the base branch from master to dev/feature September 18, 2023 10:33
@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
@sovdeeth sovdeeth added 2.9 Targeting a 2.9.X version release and removed 2.8 Targeting a 2.8.X version release labels Jan 2, 2024
@APickledWalrus APickledWalrus removed the 2.9 Targeting a 2.9.X version release label Jul 2, 2024

static {
if (Skript.classExists("org.bukkit.entity.Breedable"))
Skript.registerEffect(EffLockAge.class, "(lock|:unlock) age of %livingentities%");
Copy link
Member

Choose a reason for hiding this comment

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

It would be good to support syntax like allow entities to age or prevent entities from aging

private static final boolean HAS_MOB_SUPPORT = Skript.isRunningMinecraft(1, 16, 5);

static {
register(CondIsAdult.class, "[an] adult", "livingentities");
Copy link
Member

Choose a reason for hiding this comment

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

maybe worth including a child option?

@RequiredPlugins("MC 1.16.5+ (Mobs)")
public class CondIsAdult extends PropertyCondition<LivingEntity> {

// This is required since before 1.16 the 'isAdult' method only supported Animals
Copy link
Member

Choose a reason for hiding this comment

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

does it throw an error or something when called on a non Animals mob?

Copy link
Contributor Author

@Fusezion Fusezion Jul 19, 2024

Choose a reason for hiding this comment

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

My initial worry of this from what I see is unfounded, the same goes for the effect version, the ageable instance check already handles this, I just needed to make mentions in the description annotation


@Name("Make Breedable")
@Description({
"Picks whether or not entities will be able to breed.",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Picks whether or not entities will be able to breed.",
"Sets whether or not entities will be able to breed.",

.requiredPlugins("MC 1.16+");
}

Skript.registerEvent("Player Pickup Arrow", SimpleEvent.class, PlayerPickupArrowEvent.class, "[player] (pick[ing| ]up [an] arrow|arrow pick[ing| ]up)")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Skript.registerEvent("Player Pickup Arrow", SimpleEvent.class, PlayerPickupArrowEvent.class, "[player] (pick[ing| ]up [an] arrow|arrow pick[ing| ]up)")
Skript.registerEvent("Player Pickup Arrow", SimpleEvent.class, PlayerPickupArrowEvent.class, "[player] (pick[ing| ]up [an] arrow|arrow pick[ing| ]up)")

@@ -733,7 +735,25 @@ public class SimpleEvents {
.requiredPlugins("Paper 1.16+");
}

Skript.registerEvent("Player Pickup Arrow", SimpleEvent.class, PlayerPickupArrowEvent.class, "[player] (pick[ing| ]up [an] arrow|arrow pick[ing| ]up)")
Skript.registerEvent("Entity Breed", SimpleEvent.class, EntityBreedEvent.class, "[entity] breed[ing]")
Copy link
Member

Choose a reason for hiding this comment

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

I think it could be a good idea for these events to support triggering conditions (e.g. on pig breed) using a dedicated event class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, not a bad idea, I'll need to relook at this PR again soon, with all the changes recently there's bound to be one or two conflicts in addition to all the changes regarding conventions

Comment on lines +46 to +50
Skript.registerExpression(ExprBreedingFamily.class, LivingEntity.class, ExpressionType.SIMPLE,
"breed[ing] mother",
"breed[ing] father",
"[bred] (offspring|child)",
"breeder");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Skript.registerExpression(ExprBreedingFamily.class, LivingEntity.class, ExpressionType.SIMPLE,
"breed[ing] mother",
"breed[ing] father",
"[bred] (offspring|child)",
"breeder");
Skript.registerExpression(ExprBreedingFamily.class, LivingEntity.class, ExpressionType.SIMPLE,
"[the] breed[ing] mother",
"[the] breed[ing] father",
"[the] [bred] (offspring|child)",
"[the] breeder");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts about removing the optional [ing] in favor of forcing it; for mother and father?
I don't like breed father or the breed father

Comment on lines +102 to +105
return CollectionUtils.array(ExprExperience.class, Integer.class);
case ADD:
case REMOVE:
return CollectionUtils.array(ExprExperience[].class, Integer[].class);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return CollectionUtils.array(ExprExperience.class, Integer.class);
case ADD:
case REMOVE:
return CollectionUtils.array(ExprExperience[].class, Integer[].class);
return CollectionUtils.array(Experience.class, Integer.class);
case ADD:
case REMOVE:
return CollectionUtils.array(Experience[].class, Integer[].class);

This isn't supposed to be ExprExperience, right?

public Class<?>[] acceptChange(ChangeMode mode) {
switch (mode) {
case SET:
case RESET:
Copy link
Member

Choose a reason for hiding this comment

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

maybe worth support delete as in clear love time?

Copy link
Contributor Author

@Fusezion Fusezion left a comment

Choose a reason for hiding this comment

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

Looking over this PR again, and I want some feedback on a few things before I ship anything

Comment on lines +46 to +50
Skript.registerExpression(ExprBreedingFamily.class, LivingEntity.class, ExpressionType.SIMPLE,
"breed[ing] mother",
"breed[ing] father",
"[bred] (offspring|child)",
"breeder");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thoughts about removing the optional [ing] in favor of forcing it; for mother and father?
I don't like breed father or the breed father

Comment on lines +50 to +52
if (Skript.classExists("org.bukkit.entity.Breedable"))
Skript.registerEffect(EffBreedable.class, "make %livingentities% [negate:(not |non(-| )|un)]breedable");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm personally not a huge fan of not breedable do we believe it's fine to remove as I don't think of it as valid

Copy link
Member

Choose a reason for hiding this comment

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

go for it

Copy link
Member

@sovdeeth sovdeeth Jul 19, 2024

Choose a reason for hiding this comment

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

personally i like prevent %entities% from breeding
or the more risque sterilize %entities%

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe my latest build does add prevent and allow formatting, sterilize is possible but I'll probably put it on the back burner for now.

@sovdeeth
Copy link
Member

@Efnilite will be taking this over

@Efnilite Efnilite self-assigned this Sep 22, 2024
@Efnilite Efnilite mentioned this pull request Sep 22, 2024
@Efnilite Efnilite closed this Sep 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking changes Pull or feature requests that contain breaking changes (API, syntax, etc.) feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants