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

Add LootTableEvents.LOADED event #3352

Merged
merged 4 commits into from
Oct 8, 2023

Conversation

LLytho
Copy link
Contributor

@LLytho LLytho commented Sep 30, 2023

Hey,

The current loot table mixin https://github.com/FabricMC/fabric/blob/1.20.1/fabric-loot-api-v2/src/main/java/net/fabricmc/fabric/mixin/loot/LootManagerMixin.java#L55 updates the return value which will automatically blocks all modded mixins with the same injection but higher priority.

So I added an event which can be used to track down when all loot tables are loaded + modified by fabric events.
My first idea was to move the mixin to apply again like in previous versions but the current mixin requires the ResourceManager which does not exist in apply and I wasn't sure if collecting the ResourceManager first, storing it in the class and passing it later on is a preferable way for fabric.

@apple502j apple502j added the enhancement New feature or request label Sep 30, 2023
@apple502j apple502j requested review from modmuss50, Juuxel and a team September 30, 2023 17:55
@modmuss50
Copy link
Member

General question, whats the use case for this event? I see the test mod is only checking that loot table is not empty, I imagine you plan to do more with it?

@apple502j
Copy link
Contributor

@modmuss50 Very likely yes.

@LLytho
Copy link
Contributor Author

LLytho commented Oct 1, 2023

General question, whats the use case for this event? I see the test mod is only checking that loot table is not empty, I imagine you plan to do more with it?

Yes. I want to add loot table modification to LootJS and I want to trigger the LootJS event when all loot tables are loaded and modified by mods. So packdevs can further modify them by their needs.

* @param resourceManager the server resource manager
* @param lootManager the loot manager
*/
void onLootTableLoaded(ResourceManager resourceManager, LootManager lootManager);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
void onLootTableLoaded(ResourceManager resourceManager, LootManager lootManager);
void onLootTablesLoaded(ResourceManager resourceManager, LootManager lootManager);

technically there's multiple of them? :P

Copy link
Contributor

@maityyy maityyy Oct 1, 2023

Choose a reason for hiding this comment

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

Maybe then this event should be called ALL_LOADED? Previous events are called per loot table, so the new event may be confusing.

Also I think this event should be after the MODIFY/REPLACE events in the code, but that does not matter, it is about checkstyle.

Btw, this event is very similar to CommonLifecycleEvents.TAGS_LOADED

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also I think this event should be after the MODIFY/REPLACE events in the code, but that does not matter, it is about checkstyle.

wdym by that?

Copy link
Contributor

Choose a reason for hiding this comment

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

wdym by that?

Ah, sorry, nevermind. I am inattentive

@modmuss50 modmuss50 added the last call If you care, make yourself heard right away! label Oct 5, 2023
@modmuss50 modmuss50 added the merge me please Pull requests that are ready to merge label Oct 8, 2023
@modmuss50 modmuss50 merged commit 96dfa95 into FabricMC:1.20.1 Oct 8, 2023
5 checks passed
modmuss50 pushed a commit that referenced this pull request Oct 8, 2023
* Implement `LootTableEvents.LOADED` event

* Update for checkstyle

* rename event

* Update fabric-loot-api-v2/src/main/java/net/fabricmc/fabric/api/loot/v2/LootTableEvents.java

Co-authored-by: Juuz <[email protected]>

---------

Co-authored-by: modmuss <[email protected]>
Co-authored-by: Juuz <[email protected]>
(cherry picked from commit 96dfa95)
modmuss50 pushed a commit that referenced this pull request Oct 8, 2023
* Implement `LootTableEvents.LOADED` event

* Update for checkstyle

* rename event

* Update fabric-loot-api-v2/src/main/java/net/fabricmc/fabric/api/loot/v2/LootTableEvents.java

Co-authored-by: Juuz <[email protected]>

---------

Co-authored-by: modmuss <[email protected]>
Co-authored-by: Juuz <[email protected]>
(cherry picked from commit 96dfa95)
(cherry picked from commit 3ba460f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request last call If you care, make yourself heard right away! merge me please Pull requests that are ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants