search

MovingBlocks / Terasology

Terasology - open source voxel world
http://terasology.org
Apache License 2.0
3.64k stars 1.32k forks source link

Add Dependency Injection Support for WorldGen Plugins #5212

Open spookynutz opened 4 months ago

spookynutz commented 4 months ago

Hello,

I'm opening this PR to work on the implementation of dependency injection for the WorldGen Plugins. This PR is opened as a draft as a base for discussion since I'm new to the code and don't quite understand how things work yet.

Contains

Implementation for proposal in #5003

spookynutz commented 4 months ago

My main interrogation was where this manager should be used. My assumption from what I found was to replace this here : https://github.com/MovingBlocks/Terasology/blob/ae83461fe1f6bca1b1c4706b976e49d38286066a/engine/src/main/java/org/terasology/engine/core/modes/loadProcesses/InitialiseWorld.java#L85-L86

Could anyone confirm this ?

BenjaminAmos commented 4 months ago

Although, I am wondering if it would be easier to implement this by just calling InjectionManager.inject(item) in DefaultWorldGeneratorPluginLibrary#instantiateAllOfType: https://github.com/MovingBlocks/Terasology/blob/ae83461fe1f6bca1b1c4706b976e49d38286066a/engine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.java#L31-L45

jdrueckert commented 2 months ago

@spookynutz Did the review comments help or are you still blocked somehow? Are you planning to continue with this?

spookynutz commented 2 months ago

Although, I am wondering if it would be easier to implement this by just calling InjectionManager.inject(item) in DefaultWorldGeneratorPluginLibrary#instantiateAllOfType:

https://github.com/MovingBlocks/Terasology/blob/ae83461fe1f6bca1b1c4706b976e49d38286066a/engine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.java#L31-L45

Hello @BenjaminAmos, @jdrueckert, sorry for the delayed response I got taken by work and did not have time to advance the issue ... I'm not sure I understand correctly this comment. Are you suggesting to drop the creation of WorldGeneratorPluginManager and to rather use https://github.com/MovingBlocks/Terasology/blob/33dabf53cc675c33a2c33c193b7ec4aa1cc0174b/engine/src/main/java/org/terasology/engine/registry/InjectionHelper.java#L37 around https://github.com/MovingBlocks/Terasology/blob/33dabf53cc675c33a2c33c193b7ec4aa1cc0174b/engine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.java#L40 ?

BenjaminAmos commented 1 month ago

Are you suggesting to drop the creation of WorldGeneratorPluginManager and to rather use

https://github.com/MovingBlocks/Terasology/blob/33dabf53cc675c33a2c33c193b7ec4aa1cc0174b/engine/src/main/java/org/terasology/engine/registry/InjectionHelper.java#L37 around https://github.com/MovingBlocks/Terasology/blob/33dabf53cc675c33a2c33c193b7ec4aa1cc0174b/engine/src/main/java/org/terasology/engine/world/generator/plugin/DefaultWorldGeneratorPluginLibrary.java#L40

Yes, that is essentially what I think I meant (it was a while ago now). Although the current code is rather guilty of this already, I would prefer that new code does not introduce unecessary abstractions (the less code the better, prioritising readability).