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

Mod initialization functions #141

Merged
merged 6 commits into from
Sep 15, 2015
Merged

Mod initialization functions #141

merged 6 commits into from
Sep 15, 2015

Conversation

calclavia
Copy link
Member

Looking at the example code.

Any class which implements Loadable has three methods, preInit(), init(), and postInit().

I think this is a bad design choice for a couple of reasons:

  • Seems to just be copied from Minecraft Forge
  • Undescriptive of what's supposed to go where
  • Why allow doing the same thing in different places (block initialization in preInit or init?)
  • Unknown state of other mods at different points (recipes added in init or postInit?)
  • What would need to go into initialization code that couldn't be automated?

There should either be methods for handling specific initialization code (config, blocks, items, recipes, ...) or even have it done automatically using reflection, and let the wrapper decide the proper time to do everything.

@RX14
Copy link
Contributor

RX14 commented Jul 24, 2015

I think we should have a simple, descriptive register() method in which you register everything. Mod hooks should be achieved by interfaces on DI, but if you really want, you should be able to register a callback when a particular mod is loaded. Or maybe even specify dependencies and make sure dependant mods are loaded in order.

@Caellian
Copy link

Maybe creating an interface for mod dependencies handlers. That way you
could use a multimap [modId -> class extending interface] from guava to
allow mods to add their own handlers extending that interface.

NOVA would check the hashmap if it contains any classes bound to mod id,
and before and after loading the mod, NOVA would call corresponding methods
for every element of returned set.

That way a mod dev could write something like:

DependencyHandler.bindModHandler("thaumcraft", new IModHandler(/*Maybe mod
instance and some useful data about mods*/){
    beforeLoad(){
        // Do stuff before loading the mod
    }

    afterLoad(){
    // Do stuff after loading the mod
    }
});

Dependencies should be handled in method called for every mod before
anything else.

So in mod annotated class a method:

initDependencies(){
    DependencyHandler.handleSelfDependencies("mymodid", mods /* ArrayList<String> containing all mod id's */);
}

These are in no way ideal and their names should probably changed. Also, I
used DependencyHandler class for examples as I'm unfamiliar with NOVA
structure.
I'm sorry if this is poorly formated, I tried my best.

@calclavia
Copy link
Member

I agree that the initialization stages are ambiguous. The thing we have to allow is for cases as follows:

Mod B uses Mod A block as recipe
Mod A registers block

This will fail if Mod B loads after A.

One option I propose is to lazy evaluate everything. In other words, all registration is stored and not used until the game needs it.

@RX14
Copy link
Contributor

RX14 commented Jul 24, 2015

Well, in the register methods you are telling the NOVA Wrapper what to register, instead of when. I would say register runs before forge preInit, and then NOVA registers stuff to forge at the appropriate time.

@calclavia
Copy link
Member

@RX14 agree. And actually we might as well make registration happen in constructor, since it also prevent init being called twice...

@RX14
Copy link
Contributor

RX14 commented Jul 24, 2015

Shovelling everything into a constructor sounds strange...

@RX14
Copy link
Contributor

RX14 commented Jul 24, 2015

On the other hand, mods are less likely to have to store refs to the BlockManager etc.

@calclavia
Copy link
Member

@RX14 Yeah I agree. Will think into that because then all mod classes will be one big constructor :)

I also want to try make mod construction less annotation based.

@calclavia calclavia added this to the v0.2.0 milestone Jul 25, 2015
@calclavia calclavia self-assigned this Jul 25, 2015
@calclavia calclavia added the enhancement Nice to have features label Jul 25, 2015
@gjgfuj
Copy link
Contributor

gjgfuj commented Jul 25, 2015

And I mean, ain't nothing stopping ya from calling alternate methods in a
constructor.

On Sat, 25 Jul 2015 2:11 pm Calclavia notifications@github.com wrote:

@RX14 https://github.com/RX14 Yeah I agree. Will think into that
because then all mod classes will be one big constructor :)


Reply to this email directly or view it on GitHub
#141 (comment)
.

@RX14
Copy link
Contributor

RX14 commented Jul 25, 2015

I still think it's possible to scan the classpath for stuff to register, and register it. That cuts down on the registration we need to do. Factories etc. still needed for custom args.

@RX14 RX14 added the ready This is ready to be worked on label Jul 25, 2015
@calclavia
Copy link
Member

It's still nicer to have more control and less "magic" happening. I prefer to rely less on ASM.

@gjgfuj
Copy link
Contributor

gjgfuj commented Jul 25, 2015

Yeah, it should be possible completely to generate block types dynamically.
Not to preregister things. What about integration based registers?

On Sat, 25 Jul 2015 9:46 pm Calclavia notifications@github.com wrote:

It's still nicer to have more control and less "magic" happening. I prefer
to rely less on ASM.


Reply to this email directly or view it on GitHub
#141 (comment)
.

@RX14
Copy link
Contributor

RX14 commented Jul 25, 2015

I never said get rid of the current register system, but to autodetect things that could be registered and register them. Maybe still a bit too much magic.

Integration-based registers?

@gjgfuj
Copy link
Contributor

gjgfuj commented Jul 25, 2015

Like registering a block that only exists if another mod exists.

On Sat, 25 Jul 2015 9:51 pm Chris Hobbs notifications@github.com wrote:

I never said get rid of the current register system, but to autodetect
things that could be registered and register them. Maybe still a bit too
much magic.

Integration-based registers?


Reply to this email directly or view it on GitHub
#141 (comment)
.

@RX14
Copy link
Contributor

RX14 commented Jul 25, 2015

That would be in the mod post init hooks that I mentioned before.

@gjgfuj
Copy link
Contributor

gjgfuj commented Jul 25, 2015

Fine.

On Sat, 25 Jul 2015 9:52 pm Chris Hobbs notifications@github.com wrote:

That would be in the mod post init hooks that I meantioned befoer.


Reply to this email directly or view it on GitHub
#141 (comment)
.

@calclavia
Copy link
Member

Adding a note here for when I implement this:

  • Mod construction should happen in coremod phase in Minecraft. This allows proper library/asset downloading.
  • Use less annotation based loading

@calclavia calclavia changed the title Regarding mod initialization functions Mod initialization functions Jul 28, 2015
@calclavia calclavia modified the milestones: v0.1.0, v0.2.0 Jul 29, 2015
@asiekierka
Copy link
Contributor

Bleh.

My proposal: add a way for mods to specify arbitrary loading stages.

Let's say NOVA-Core adds an "initializeBlocks" stage. If you were to initialize blocks, then, you'd create a loading stage "MyMod:blocks" which happens before "initializeBlocks".

That way, you could also introduce arbitrary loading stages which happen nowhere in particular, like "BuildCraft:initializeBlueprintSchematics". Then another mod could make their own loading state which happens right after that. And create a dependency tree out of it which allows for a lot more granularity than anything else.

It might be slightly overengineered, however. Needs a way to make it user-friendly.

@copygirl
Copy link
Author

@asiekierka That could be easily done with an annotation, I guess. For example:

@Initialize("blocks", "nova:blocks") // or
@Initialize("blocks", "nova:blocks, after=OtherMod:blocks") // or
@Initialize("blocks", "nova:blocks", after="OtherMod:blocks") // or
@Initialize("blocks", "after=OtherMod:blocks") // This would throw a dependency error if
                                               // OtherMod:blocks doesn't actually exist.
public void initializeModBlocks() { ... }

Where this function could be referenced as <modid>:blocks.

Though I'm not sure I would support this way of doing things. You could end up with some pretty nasty dependency problems, because modders could arbitrarily choose to do things whenever. Registering everything at construction and then actually initializing things when it seems most fitting for the wrapper seems to make the most sense to me.

@RX14
Copy link
Contributor

RX14 commented Jul 29, 2015

What advantages does this have over the previous single stage loading that I suggested. It simply seems more complex to me.

@asiekierka
Copy link
Contributor

@RX14 - more granularity, I guess. What if you need to replace a block after it is registered? Etc.

It's just an idea.

@gjgfuj
Copy link
Contributor

gjgfuj commented Jul 29, 2015

This seems pretty nice imo, along with the previous register all before
then wrapper does that.

On Wed, 29 Jul 2015 7:12 pm Adrian Siekierka notifications@github.com
wrote:

@RX14 https://github.com/RX14 - more granularity, I guess. What if you
need to replace a block after it is registered? Etc.

It's just an idea.


Reply to this email directly or view it on GitHub
#141 (comment)
.

@calclavia
Copy link
Member

@copygirl We could (instead of using annotations), have methods that have the corresponding manager arguments.

void initBlocks(BlockManager manager)

Then again, I'm not a big fan of using lots of annotations. In fact, I'm trying to avoid too many annotations as they voids any possibly to do things dynamically (also because it's going to be painful to implement annotations in NOVA JS). We could simply do method registration into the GlobalEvent handler, that gets called when the corresponding init stage is required.

events.on(BlockInit.class).bind(this::initBlocks)

@RX14
Copy link
Contributor

RX14 commented Jul 30, 2015

@Victorious3 all I am saying is for managers to have an eventbus too, with registration events. The event might be cancellation for very easy, explicit, functional style registration filtering.

@calclavia calclavia modified the milestones: v0.2.0, v0.1.0 Jul 31, 2015
@calclavia calclavia modified the milestones: v0.1.0, v0.2.0 Sep 10, 2015
@calclavia
Copy link
Member

So my decision is to try lazy loading, see if that works well.

PROGRESS:

  • Implement lazy loading
  • Handle dependencies

TODO: #196

@calclavia
Copy link
Member

Suggestion: We can do named event handlers.

events
  .on(BlockEvent.Register.class)
  .withName(this.modID)
  .before(otherModID)
  .after(otherModID)
  .after(otherModID)
  .bind(this::registerBlocks);

This method of registration is enforced as such: Methods to register blocks will only exist on the block register event.

@RX14
Copy link
Contributor

RX14 commented Sep 15, 2015

@calclavia this is good base stuff, but having a more simple abstraction over this would be nice.

@calclavia
Copy link
Member

@RX14 Such as? Any example code you have in mind?

@RX14
Copy link
Contributor

RX14 commented Sep 15, 2015

No idea, sorry. Implement this for now, and we can decide on better abstractions later.

@copygirl
Copy link
Author

What about the lazy initialization idea?

@calclavia
Copy link
Member

@copygirl It's difficult to handle dependencies with lazy init...

Lazy init example code for your mod's main class:

public ModName(BlockManager blockManager){
  blockManager.register(...);
  blockManager.register(...);
  blockManager.register(...);
}

After that, NOVA handles when the blocks are actually registered with Forge.

How do we handle if mod A wants to register blocks after mod B? I feel like the event system is nicer, unless you can come up with a good suggestion.

@ghost
Copy link

ghost commented Sep 15, 2015

👍 for named event stuff

@calclavia
Copy link
Member

@anti344 Working on it right now... Topological sorting doesn't seem lightweight.

@codecov-io
Copy link

Current coverage is 13.33%

Merging #141 into master will increase coverage by +0.41% as of 41efd8a

@@            master    #141   diff @@
======================================
  Files          391     393     +2
  Stmts        10437   10500    +63
  Branches      1490    1487     -3
  Methods          0       0       
======================================
+ Hit           1349    1400    +51
+ Partial         75      73     -2
- Missed        9013    9027    +14

Review entire Coverage Diff as of 41efd8a

Powered by Codecov. Updated on successful CI builds.

calclavia added a commit that referenced this pull request Sep 15, 2015
Improve mod initialization functions
@calclavia calclavia merged commit 1eb9200 into master Sep 15, 2015
@calclavia calclavia removed the ready This is ready to be worked on label Sep 15, 2015
@@ -91,4 +95,86 @@ public void testRemovalByObject() {

assertThat(event.toString()).isEqualTo("A");
}

@Test
public void testNamedPriority1() {
Copy link
Contributor

Choose a reason for hiding this comment

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

testNamedPriorityBefore

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Nice to have features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants