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 support for itemstacks in item cooldown expression/condition #7707

Open
wants to merge 14 commits into
base: dev/feature
Choose a base branch
from

Conversation

Fusezion
Copy link
Contributor

Description

This PR aims to add support for itemstacks for the cooldown expression and condition. This change adds support for the cooldown group of itemstacks.

This PR does not add support for setting a cooldown group, that is left for when components are handled/when someone implements paper's components

With the implementation of itemstacks I've decided to change how they were grabbed and handled, in addition with the design of the system I was not 100% sure if using getMaterial properly support all x when people still had aliases

The new system runs them through a map for getAll and then follows it with a flat map to an itemstack list

Why no Test?

Test were not included in this PR as I am not sure how to go around implementing them, as I don't believe junit would be able to actually run a proper test on a fake player.


Target Minecraft Versions: 1.20.5+
Requirements: none
Related Issues: #7429

Copy link
Contributor

@Absolutionism Absolutionism left a comment

Choose a reason for hiding this comment

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

Only concern I have with this is the possible breaking changes.
If a user who is currently running a server that supports cooldown groups and is utilizing the current functionality, they would need to fix/update their code to retain how they're already using it.
It's probably safe to assume that majority of users would prefer the itemstack+cooldown group over the item type.
But, I think it would be nice if there was a way for users to use either or.

@Fusezion
Copy link
Contributor Author

Only concern I have with this is the possible breaking changes. If a user who is currently running a server that supports cooldown groups and is utilizing the current functionality, they would need to fix/update their code to retain how they're already using it. It's probably safe to assume that majority of users would prefer the itemstack+cooldown group over the item type. But, I think it would be nice if there was a way for users to use either or.

The only breaking change for servers is if they define cooldown groups and expect it to work with current functionality, if the component is not defined or no group is defined on the component there is no breaking change

@Absolutionism
Copy link
Contributor

The only breaking change for servers is if they define cooldown groups and expect it to work with current functionality, if the component is not defined or no group is defined on the component there is no breaking change

Ahh ok, so using #hasCooldown(ItemStack) internally checks if there is an assigned cooldown group and if not, defaults to itemtype?

@Fusezion
Copy link
Contributor Author

The only breaking change for servers is if they define cooldown groups and expect it to work with current functionality, if the component is not defined or no group is defined on the component there is no breaking change

Ahh ok, so using #hasCooldown(ItemStack) internally checks if there is an assigned cooldown group and if not, defaults to itemtype?

Internally it would check if there's a cooldown group
if yes -> set cooldown for the group
if no -> set cooldown for the material

@Absolutionism
Copy link
Contributor

Alrighty, in that case

@Burbulinis Burbulinis added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Mar 19, 2025
@Fusezion Fusezion requested a review from Efnilite March 21, 2025 01:59
@Fusezion Fusezion requested a review from a team as a code owner March 21, 2025 11:02
@Fusezion Fusezion requested review from sovdeeth and removed request for a team March 21, 2025 11:02
@Fusezion Fusezion requested a review from sovdeeth March 24, 2025 02:07
Copy link
Member

@Efnilite Efnilite left a comment

Choose a reason for hiding this comment

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

the annotation description still doesnt explain what a cooldown group is

)
);
return players.check(event, (player) -> {
return SimpleExpression.check(itemTypes.getArray(event), itemType -> {
Copy link
Member

Choose a reason for hiding this comment

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

need to getArray outside the check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature request, an issue about something that could be improved, or a PR improving something.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants