Skip to content

Global Loot Funcs on 1.15.x#6401

Merged
LexManos merged 11 commits intoMinecraftForge:1.15.xfrom
Draco18s:Global-Loot-Funcs-1.15
Feb 27, 2020
Merged

Global Loot Funcs on 1.15.x#6401
LexManos merged 11 commits intoMinecraftForge:1.15.xfrom
Draco18s:Global-Loot-Funcs-1.15

Conversation

@Draco18s
Copy link
Copy Markdown
Contributor

@Draco18s Draco18s commented Jan 1, 2020

See #6267 for complete details.

Data driven system for arbitrary loot modification as a replacement for HarvestDropsEvent, addressing #5871.

@Unnoen Unnoen added 1.15 Assigned This request has been assigned to a core developer. Will not go stale. Feature This request implements a new feature. labels Jan 1, 2020
@metelares
Copy link
Copy Markdown

metelares commented Jan 1, 2020

change e-mail

@Draco18s
Copy link
Copy Markdown
Contributor Author

Draco18s commented Jan 2, 2020

Hey @LexManos just saw the LTS post on the forums, your request for me to target 1.15 in order to get this merged makes a lot more sense now. 👍 Just let me know if anything needs to be changed and I'll get it done asap.

@metelares huh? Your comment makes no sense.

LexManos
LexManos requested changes Jan 7, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@LexManos LexManos left a comment

Choose a reason for hiding this comment

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

This isn't bad, my main concerns are the lack of documentation on the root internfaces.
I also don't see where the serialization comes into play.
And lastly, one of the main reasons I've been procrastinating doing this. Do we just not care about reentrancy at all?

@Draco18s
Copy link
Copy Markdown
Contributor Author

Draco18s commented Jan 7, 2020 •
edited
Loading

Other comments re @LexManos

I also don't see where the serialization comes into play.

It probably doesn't. Any code that still exists as a vestige of my hacking about semi-blindly. I'm not even sure what parts that might be, the interface only contains a read method, no write. If I missed something, point it out.

And lastly, one of the main reasons I've been procrastinating doing this. Do we just not care about reentrancy at all?

I don't know, do we? This is one of the reasons I tried to request feedback from you two months ago when I started working on this. The only comment in this regard was about what happens if two mods want to modify the same drop differently, what order takes precedence, which is why the list is sorted by registry name.

If we do care, we also have to address A->B + A->C precedence handling, not just B->C + A->B. The alternative is to not care, which has precedent (there can not be an "absolutely last, no matter what" event handler).

I wrote this PR because no one else was, I don't know what I'm doing, I'm just trying to get something done so that I (and many others) can move forwards.

@Draco18s
Updates based on Lex's comments
e472194 
Removed @author
Added a LootContext.Builder constructor that can clone a context object
Added javadoc
Made LootModifierManager not a singleton
@Draco18s
Missing json files
9b0acd4
@Draco18s
Refactor to read layered config file
6807fd8 
Reads all.json to determine what loot modifiers to load. Functions like tags.
@LexManos LexManos added Work In Progress This request has lots of changes that need attention. and removed Assigned This request has been assigned to a core developer. Will not go stale. labels Jan 16, 2020
@LexManos LexManos removed their assignment Jan 16, 2020
@LexManos
Copy link
Copy Markdown
Member

LexManos commented Jan 16, 2020

We had a long discussion and brainstorming session on discord, he knows what he needs to do to move forward. Marked this as WIP until he's done.

@Draco18s
Further refactor
444deb4 
Updated EntityLiving's call
Updated LootTable to mark previous entityliving entry as deprecated
Renamed all.json to global_loot_modifiers.json
Relocated test mod data resources to generated_test
Fixed setRegistryName implementation
Tabs to spaces
Refactored json loading system making it simpler and less round about.
@Draco18s
Docs clean
3c18f04 
Cleaned up javadoc wording in several places
Made setRegistryName final
@Draco18s
Wordsmithing
92bf134 
Adjusted javadoc, added @nonnull annotations
@diesieben07 diesieben07 mentioned this pull request Jan 21, 2020
Closed
@Draco18s
Merge branch '1.15.x' into Global-Loot-Funcs-1.15
0b402ce 
# Conflicts:
#	patches/minecraft/net/minecraft/entity/LivingEntity.java.patch
#	patches/minecraft/net/minecraft/server/MinecraftServer.java.patch
#	src/test/resources/META-INF/mods.toml
@Draco18s Draco18s requested a review from LexManos January 31, 2020 01:47
@Darkere Darkere mentioned this pull request Feb 7, 2020
Open
@Draco18s
Copy link
Copy Markdown
Contributor Author

Draco18s commented Feb 15, 2020

All requested changes have been made. I have requested a re-review. What is holding this up?

gigaherz
gigaherz reviewed Feb 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@gigaherz gigaherz left a comment

Choose a reason for hiding this comment

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

I had completely disregarded this PR thinking it was just some other "let's add new loot events!" proposal, and I'm pleasantly surprised of what I see.

I do have one concern, but don't think of it as a change request, rather only as a discussion topic.

@gigaherz gigaherz removed the Work In Progress This request has lots of changes that need attention. label Feb 19, 2020
@Draco18s
Changed from Interface to Abstract Class
e07b29b 
GlobalLootModifierSerializer is now an abstract class and the abstract implementation in LootModifier was no longer needed.
@Draco18s
Stray tab
d4b6540
LexManos
LexManos approved these changes Feb 27, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@LexManos LexManos left a comment

Choose a reason for hiding this comment

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

Few code style issues, and still lacking documentation in places. But it'll work.

@LexManos LexManos merged commit 58e5f33 into MinecraftForge:1.15.x Feb 27, 2020
@Draco18s
Copy link
Copy Markdown
Contributor Author

Draco18s commented Feb 27, 2020

I'll backport this to 1.14 this weekend. I'll see if I can do something about docs and style, but I feel like I've been over them a bunch and am not sure what to add.

@Draco18s Draco18s mentioned this pull request Mar 1, 2020
Closed
This was referenced Apr 12, 2020
Closed
Closed
@dhharris
Copy link
Copy Markdown

dhharris commented Apr 25, 2020 •
edited
Loading

@Draco18s , I'm looking for a HarvestDropsEvent replacement so I can keep track of loot mined. Does this feature support that?
I'm looking to track loot without modifying it. For example, you mine a redstone ore, and during the event it updates a class with the BlockPos and number of redstone dropped.

@Draco18s
Copy link
Copy Markdown
Contributor Author

Draco18s commented Apr 25, 2020

You'd still have to register it as a modifier, but you don't have to actually modify anything (and you'd still get the list of harvested loot and full context).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@LexManos LexManos LexManos approved these changes

+1 more reviewer

@gigaherz gigaherz gigaherz left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@LexManos LexManos

Labels

1.15 Feature This request implements a new feature.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@Draco18s @metelares @LexManos @dhharris @gigaherz @Unnoen