Skip to content

Add ConsumeEffects #2585

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

Open
wants to merge 10 commits into
base: api-14
Choose a base branch
from

Conversation

MrHell228
Copy link
Contributor

@MrHell228 MrHell228 commented Mar 28, 2025

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 force-pushed the api-14-consume-effects branch from 5f0a59e to c0b74b5 Compare March 28, 2025 16:25
@MrHell228
Copy link
Contributor Author

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
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
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants