Skip to content

Add ItemActions#2585

Closed
MrHell228 wants to merge 13 commits intoSpongePowered:api-14from
MrHell228:api-14-consume-effects
Closed

Add ItemActions#2585
MrHell228 wants to merge 13 commits intoSpongePowered:api-14from
MrHell228:api-14-consume-effects

Conversation

@MrHell228
Copy link
Copy Markdown
Contributor

@MrHell228 MrHell228 commented Mar 28, 2025 •
edited
Loading

SpongeAPI | Sponge

Exposes ConsumeEffects, addsKeys.CONSUME_EFFECTS and Keys.DEATH_PROTECTION_EFFECTS to apply them to ItemStack.

Currently Keys.APPLICABLE_POTION_EFFECTS does nothing and its WeithedTable approach doesn't make much sense anymore so I think it would be better to just delete it.

@MrHell228 MrHell228 mentioned this pull request Mar 28, 2025
Closed
@MrHell228 MrHell228 force-pushed the api-14-consume-effects branch from 5f0a59e to c0b74b5 Compare March 28, 2025 16:25
@MrHell228
Copy link
Copy Markdown
Contributor Author

MrHell228 commented Mar 30, 2025

Actually, ConsumeEffect might be not the best name because DEATH_PROTECTION component also uses them. Can't think of a different name though

@aromaa
Copy link
Copy Markdown
Member

aromaa commented Mar 31, 2025

More fitting names could be ItemActionEffect or EntityActionEffect. I'm not really sure would we like to actually generalize this and then Mojang throws a curveball at us but food for thought.

@MrHell228 MrHell228 force-pushed the api-14-consume-effects branch from e70c4d9 to e0debb6 Compare March 31, 2025 02:29
@MrHell228
Copy link
Copy Markdown
Contributor Author

MrHell228 commented Mar 31, 2025

ItemActionEffect sounds fine, changed. Also removed World arg for ItemActionEffect#apply since it's only used for sounds and not really excepted to differ from entity world.
I'm not sure what you mean by not generalizing this. If it's about having like Keys<List<Double>> and Key<List<SoundType>> for teleports and sounds, then there is still no guarantee that mojang won't make it, for example, single teleport and single sound effect at some point.
So I guess nothing can be considered as a totally right choice for future here.

@Yeregorix Yeregorix changed the title Add ConsumeEffects Add ItemActionEffects Sep 15, 2025
Yeregorix
Yeregorix reviewed Sep 15, 2025
View reviewed changes
Comment thread src/main/java/org/spongepowered/api/data/Keys.java
/**
* The set of {@link PotionEffect}s applied on use of an {@link ItemStack}.
*/
public static final Key<WeightedCollectionValue<PotionEffect>> APPLICABLE_POTION_EFFECTS = Keys.weightedKey(ResourceKey.sponge("applicable_potion_effects"), PotionEffect.class);
Copy link
Copy Markdown
Member

@Yeregorix Yeregorix Sep 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Because API-14 have been released, this would be a breaking change. Rebase the PR to API-15 or keep this key for now and remove it in later versions.

@Yeregorix
Copy link
Copy Markdown
Member

Yeregorix commented Sep 15, 2025 •
edited
Loading

ItemActionEffect sounds a bit redundant to me. Wouldn't ItemAction or ItemEffect be enough? I have a slight preference for ItemAction as it is easier to distinguish from PotionEffect.

@MrHell228
Copy link
Copy Markdown
Contributor Author

MrHell228 commented Sep 15, 2025

I don't mind renaming it to ItemAction. I guess it does reduce the understanding of the class purpose from the name but I believe that's what docs are for

@Yeregorix
Copy link
Copy Markdown
Member

Yeregorix commented Sep 15, 2025

Well, that's just my opinion, I'm not 100% sure

@MrHell228
Copy link
Copy Markdown
Contributor Author

MrHell228 commented Sep 15, 2025

I prefer shorter names overall though here I'm not 100% sure as well :/

@MrHell228
Copy link
Copy Markdown
Contributor Author

MrHell228 commented Sep 15, 2025 •
edited
Loading

Renamed to ItemAction (new keys got renamed as well) and returned deleted key (it had no implementation anyway)

@Yeregorix Yeregorix changed the title Add ItemActionEffects Add ItemActions Sep 24, 2025
Yeregorix pushed a commit that referenced this pull request Sep 24, 2025
@MrHell228 @Yeregorix
Add ItemActions (#2585)
3462203
@Yeregorix
Copy link
Copy Markdown
Member

Yeregorix commented Sep 24, 2025

Merged by 3462203

@Yeregorix Yeregorix closed this Sep 24, 2025
@MrHell228 MrHell228 deleted the api-14-consume-effects branch October 23, 2025 23:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yeregorix Yeregorix Yeregorix left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@MrHell228 @aromaa @Yeregorix