Call GetHooks only once for every Mod + README changes

Drakulix / ScrollsModLoader

A Mod Loader Framework for the game "Scrolls" by Mojang

www.scrollsguide.com/summoner

GNU Lesser General Public License v2.1

15 stars 9 forks source link

Call GetHooks only once for every Mod + README changes #3

Closed MaPePeR closed 11 years ago

MaPePeR commented 11 years ago

Works for me without any problems. Please review it.

Fixes #2

MaPePeR commented 11 years ago

Maybe it would be much better to use the MethodDefiniton-Objects as Keys to the Dictionary. That would also partly solve #5, but i'm afraid of MethodDefiniton not implementing the hash value correctly.

Also i'm happy that this seems to remove lag for Mac users.

So TODO list for this pull request:

[ ] Call GenerateHookDict at a proper place
[ ] Maybe create a separate class for the Method -> Mods assignment
[x] Check if MethodDefinition is a valid Key for a Dictionary - Its not!
[ ] Implement IEqualityComparer for MethodDefinition
[ ] Use the same thing for Patches (Whats the difference between a Patch and a BaseMod that only uses Method Replacements?)

Drakulix commented 11 years ago

The difference between a mod and a patch is, that patches have higher priority and cannot be overridden by mods. Patches should also be used only internally, they provide a way to make sure, that the base functionality of the modloader does not get altered by mods.

MaPePeR commented 11 years ago

have higher priority cannot be overridden

Isn't that the same thing? I think, a mod, that uses TargetMethod.Invoke when replacing the methods and has the highest priority should behave the same as a Patch. When joining Patch and Mod together we can simplify the Interception-Process very much...

Drakulix commented 11 years ago

So you would suggest to somehow merge them, but apply "special rules" to internal mods and set them to the highest priority?

My only problem with that is, that other mods might place an "around"-invocation around overridden functions, that used to be patches. And that may alter the behaviour of the mod loader itself, which is what I wanna avoid.

MaPePeR commented 11 years ago

It would not really be special rules. Just a mod, which does not appear in the Modlist, but has hooks assigned and is the first mod that was loaded (so highest priority)

I have no idea, what you mean by 'around'-invocation. AfterInvoke/BeforeInvoke? I think these already get executed for patched methods.

Well, that's the way i would do it. From my perspective i do not see any 'real' difference that would speak against that merge, but maybe i didn't see something.

Drakulix commented 11 years ago

they do? Then thats already not on purpose, if I think about it.

I see that there is room for improvements, but give me some time to think about, how to solve this the best way, I already have some ideas. I will tackle that subject with the next update for sure.

MaPePeR commented 11 years ago

Yes they do: See /Core/ModLoader.cs#L95 and #L149 - Not affected by replacement/patches

I think when somebody want to cause harm with Scrolls-Mods one does not need to mess up with the ModLoader to do so. :) Also implementing the Modloader-GUI as a Mod might be helpful when an update breaks the Modloader-GUI, but not the Modloader itself (i think this is very likely to happen). So it might be possible to still use mods that didn't break. - only configuring the modloading-order and enable/disable mods need to be done in filesystem then.

MaPePeR commented 11 years ago

I'm kinda unsure how you would want to implement the rest of that TODO-List i made up, so i think i will leave it like this and let you decide the further steps.

MaPePeR commented 11 years ago

I just encountered some weird behavior with this version: Some mods seem to fail the mod = loader.modInstances [modId]; (KeyNotFoundException) at Line 52, but that is weird, because the exact same line exists in the original code...

MaPePeR commented 11 years ago

5b1609e seems to fix the problem with the KeyNotFoudException and mods not being enabled.

MaPePeR commented 11 years ago

Maybe the ~~MethodDefinition.toString()~~ MethodReference.FullName() could be used to identify the methodhooks - it contains the name, the parameters and ~~i think~~ the source code indicates that the 'declaringType'-Name is in there, too.

And i just hope, that it does create the same string for the same methods...

MaPePeR commented 11 years ago

Merged master(f3f8623ad3d32a8252512c7832861776a6f0e56a) into this pull Request. (Sorry, link is wrong in the commit message)

MaPePeR commented 11 years ago

If you don't want to merge this whole pull request you might want to cherry pick 991fc54, because it fixes Summoner with Judgement.