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

In Control does not allow other mods to respond to changes it makes to the drops array during firing of LivingDropsEvent #154

Closed
sam-kirby opened this issue Sep 8, 2019 · 1 comment

Comments

@sam-kirby
Copy link

Description

In Control subscribes to the LivingDropsEvent with Lowest priority. This means any drops it adds to the list cannot necessarily be dealt with by other mods. Conversely, drops it removes may already have been handled by another mod.

Example

Astral Sorcery has a perk which causes all drops from LivingDropsEvent to go into your inventory (https://github.com/HellFirePvP/AstralSorcery/blob/1.12.2/src/main/java/hellfirepvp/astralsorcery/common/constellation/perk/tree/nodes/key/KeyMagnetDrops.java#L43-L64)

It also subscribes with Lowest priority. Forge selects Astral's handler first. Any drops in the list at this point are added to the player's inventory, including any which would later be removed by In Control.

In Control's handler is later called, there is nothing to remove at this stage so it adds drops which are then placed on the ground.

DarkPacks/SevTech-Ages#3847

Possible Resolution

Anything subscribing to the event with priority Lowest is likely doing so to consume the final List. It therefore seems reasonable that all modifications should be made before this point.

In Control is making modifications, but as these removals may depend on the actions of other mods in higher priority slots, it seems reasonable to keep it at as low priority as possible. Moving the event subscription to Low priority could be a good compromise.

@McJty
Copy link
Collaborator

McJty commented Sep 9, 2019

Whatever solution is chosen here, there will always be a potential for problems. If some other mod also uses 'Low' then In Control might conflict with that. Lowest was chosen to avoid problems with other mods in the past. This is not something I can change now as it will break compatibility. I'm afraid there is no good solution for this

@McJty McJty closed this as completed Sep 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants