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

Adds Loot Tables #7242

Merged
merged 67 commits into from
Dec 24, 2024
Merged

Conversation

Burbulinis
Copy link
Contributor

Description

Adds loot tables and loot generation to Skript.


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

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.

Good start :)

Copy link
Contributor

@Asleeepp Asleeepp left a comment

Choose a reason for hiding this comment

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

tests tests tests!!!

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.

Just to cause more mayham, wanna make this a module?

@Burbulinis Burbulinis requested a review from Moderocky December 4, 2024 15:46
@Efnilite Efnilite added the feature Pull request adding a new feature. label Dec 4, 2024
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.

After talking a bit with someone else and getting their feedback on a suggestion I wanna post it here for you as well.

What's the thought about making a wrapper class for the loot context similar to how recipes were done?

Currently every time you wanna change the syntax you end up doing new builder -> copy old stats -> add new stat for each of the stats in other words

ExprLootContextWithEntity -> new -> edit entity -> return
ExprLootContextWithKiller -> new -> edit killer -> return
ExprLootContextWithLuck   -> new -> edit luck   -> return

Due to this you ended up with two expressions for the same things
ExprLootContextWithX and ExprLootContextX these could be merged into 1 expression allowing changers and using a wrapper around LootContext with methods for setting the values you can do something like

LootContextWrapper#setKiller
LootContextWrapper#setLuck
LootContextWrapper#setEntity

Hell you could probably even change the loot context location. This addition will also allow the usage of a Secpression meaning we could instead see syntax like

set {_plank} to a new loot context:
    set loot context entity to {_player}
    set loot context luck to 10
    set loot context killer to {_players twin brother}
    set loot context location to location({_x},-5,10, "World")

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.

A lot of the syntaxes are worryingly generic. As a specific example, I can see people getting very confused with killer of {_thing}. I would prefer a lot of these to have required keywords that make sure they don't conflict/get confused with other syntaxes.

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.

Looking good, your test files need to end with new lines though :)

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.

I didn't look at changers and I removed things that mentioned to use Loottabletypes until they're added back for for a first look it's good. I really wish github didn't force line breaks on long lines

Copy link
Contributor

@TheAbsolutionism TheAbsolutionism left a comment

Choose a reason for hiding this comment

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

Slight look over.

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.

This is looking really good. Great work so far!

Copy link
Contributor

@TheAbsolutionism TheAbsolutionism left a comment

Choose a reason for hiding this comment

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

Some nitpick things

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.

Should be the final changes 🥳

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.

You have done an excellent job. Great work!!

@Burbulinis
Copy link
Contributor Author

You have done an excellent job. Great work!!

thank you pickel :)

Copy link
Contributor

@TheAbsolutionism TheAbsolutionism left a comment

Choose a reason for hiding this comment

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

Looks to be the last few things.

@Efnilite Efnilite closed this Dec 21, 2024
@Efnilite Efnilite reopened this Dec 21, 2024
@Moderocky Moderocky merged commit b123b88 into SkriptLang:dev/feature Dec 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.10 Targeting a 2.10.X version release feature Pull request adding a new feature. feature-ready A PR/issue that has been approved, tested and can be merged/closed in the next feature version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants