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

Additional Dropped Item Elements #7270

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

Conversation

TheAbsolutionism
Copy link
Contributor

Description

This PR aims to add additional elements relating to Dropped Items


Target Minecraft Versions: any
Requirements: none
Related Issues: #5110

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.

iirc skript has a type registered for dropped entities can we make use of that instead of entity here so people don't interpret differently

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Some initial thoughts

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.

Some of the same as pickle probably

@TheAbsolutionism
Copy link
Contributor Author

iirc skript has a type registered for dropped entities can we make use of that instead of entity here so people don't interpret differently

I'm not seeing any registered droppeditem or droppedentity

@Fusezion
Copy link
Contributor

iirc skript has a type registered for dropped entities can we make use of that instead of entity here so people don't interpret differently

I'm not seeing any registered droppeditem or droppedentity

it's likely itementity and would be registered with NoDocs
image

@TheAbsolutionism
Copy link
Contributor Author

it's likely itementity and would be registered with NoDocs ![image](https://private-user-

ahhh ok, appreciate it

Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

Looking better!

Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

Just a few thoughts.

Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

Just some concerns I guess.

@Efnilite Efnilite added the enhancement Feature request, an issue about something that could be improved, or a PR improving something. label Dec 17, 2024
Copy link
Contributor

@ShaneBeee ShaneBeee left a comment

Choose a reason for hiding this comment

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

I'm still not a fan of returning null when something isn't null provided by Bukkit/Minecraft.
In my opinion this is super misleading.
ie: if owner/thrower of %itementity% is set: incorrectly returning false.
But if that is what the team wants, then I'll approve.

src/main/java/ch/njol/skript/effects/EffItemLifetime.java Outdated Show resolved Hide resolved
Copy link
Member

@APickledWalrus APickledWalrus left a comment

Choose a reason for hiding this comment

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

I think this is just about ready. I'm really liking the syntax changes.

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.

Please don't hate me, mostly just concerns or questions

@ShaneBeee
Copy link
Contributor

I'm still not a fan of returning null when something isn't null provided by Bukkit/Minecraft. In my opinion this is super misleading. ie: if owner/thrower of %itementity% is set: incorrectly returning false. But if that is what the team wants, then I'll approve.

@ShaneBeee I think this is fair, but for a Skript user, I'm not sure the distinction is important. I might argue that the thrower might as well be not set if it points to a non-existent entity. I'd imagine most users that want to obtain the thrower want to do something with that entity, but perhaps I am missing a larger use case. I'd love to hear what others think.

@APickledWalrus my main issue is when using this in conjunction with ... is set: ... it will return false even tho it shouldn't be false as it IS set.

@APickledWalrus
Copy link
Member

my main issue is when using this in conjunction with ... is set: ... it will return false even tho it shouldn't be false as it IS set.

@ShaneBeee Yeah, I think that's fair. It could be confusing, though I do think it might as well be not set to the average Skript user (since they can't do anything with it). Of course, I'm operating on the assumption that it being a dead UUID means it is useless, which very well may not be the case.

@sovdeeth
Copy link
Member

I'm ok with it returning a string uuid as a last resort, but I don't think there's a good solution either way. If we return a string uuid, then uuid of (owner of x) would be null if it returns a uuid, which I think is as confusing as it not being set when it's a uuid.
I'm generally ambivalent on the two options

static {
Skript.registerEffect(EffItemDespawn.class,
"(prevent|disallow) %itementities% from (naturally despawning|despawning naturally)",
"allow natural despawning of %itementities%",
Copy link
Member

Choose a reason for hiding this comment

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

I would say there should be a corresponding
(prevent|disallow) natural despawning of %itementities%

I'm also wondering if natural should be optional, as in prevent despawning of %itementities% (not 100% sure on that)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me know

…tems

# Conflicts:
#	src/main/java/ch/njol/skript/expressions/ExprEntityTamer.java
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.

7 participants