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

Keep items expression and effect #6051

Closed
wants to merge 5 commits into from

Conversation

moraedu
Copy link

@moraedu moraedu commented Sep 21, 2023

Description

Adds the missing keep inventory effect and expressions.

Marking it as a draft since I still haven't tested it but want opinion on whether I should add a pattern to not clear the drops for the keep items effect, since that's kind of the whole reason why I added the effect in the first place so that users don't have to do it themselves.


Target Minecraft Versions: 1.13+
Requirements: Paper
Related Issues: N/A

@AyhamAl-Ali AyhamAl-Ali added the feature Pull request adding a new feature. label Sep 21, 2023
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Did a minor once through, just some nitpicks and mentions I haven't seen yet

src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
src/main/java/ch/njol/skript/effects/EffKeepItems.java Outdated Show resolved Hide resolved
@moraedu
Copy link
Author

moraedu commented Sep 24, 2023

I am planning to merge EffKeptItems to the EffKeepInventory, I did not realize there was already a keep inventory effect. Also, @Moderocky has pointed out that the syntax of both might need some rewording, so if anyone can come up with different syntaxes for them then I am all ears

@moraedu moraedu force-pushed the feature/kept-items branch from f1fb1b6 to 6bfa7be Compare October 4, 2023 06:16
@moraedu
Copy link
Author

moraedu commented Oct 4, 2023

I decided to remove the kept items expression as its behaviour was rather unclear, I could have just renamed it to respawn items or something along these lines however I'd rather have the keep specific items pattern be merged rather than have to deal with a better fitting syntax for the expression.

Copy link
Member

@Moderocky Moderocky left a comment

Choose a reason for hiding this comment

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

I still think this is a little wobbly in execution but it's ok.

Moderocky and others added 2 commits October 22, 2023 21:12
Co-authored-by: LimeGlass <16087552+TheLimeGlass@users.noreply.github.com>
@sovdeeth sovdeeth added the 2.8 Targeting a 2.8.X version release label Dec 30, 2023
Copy link
Member

@sovdeeth sovdeeth left a comment

Choose a reason for hiding this comment

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

I don't think this is implemented properly.
keep inventory maintains the items in the inventory and still has them in the drops.
keep the dirt maintains 1 dirt in the inventory and removes 1 dirt from the drops.
keep all dirt maintains 1 dirt in the inventory and removes it all from the drops

I'd like to see parity between behavior when keeping inventory and keeping specific items, and I think work needs to be done to handle all better.
I'm also of the opinion that keep dirt should keep ALL dirt, and keep 1 dirt should only keep 1 dirt, but I'm not sure if that's feasible.

@sovdeeth sovdeeth removed the 2.8 Targeting a 2.8.X version release label Dec 31, 2023
@sovdeeth
Copy link
Member

Closing due to inactivity.

@sovdeeth sovdeeth closed this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Pull request adding a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants