CitiesSkylinesMods / TMPE

Cities: Skylines Traffic Manager: President Edition
https://steamcommunity.com/sharedfiles/filedetails/?id=1637663252
MIT License
571 stars 85 forks source link

Migration to Harmony #119

Closed krzychu124 closed 5 years ago

krzychu124 commented 5 years ago

As we know there are serious problems with overriding our methods by other mods.

@VictorPhilipp did great job with implementation in separate branch. It is more than 5 patches behind of our current version (about 140 commits :smile: ) but it has some other changes to citizen manager, Flags and other things that looks promissing but not released yet. I have to test if everything it in place and 'working'.

I will try to merge our latest changes and push it to new branch, probably 1.11.0-pre-alpha to give you opportunity to do more tests on different environments, check compatibility with savegames(loading/saving) etc.

Maybe I'll even add compiled dlls to speed up testing.

Strdate commented 5 years ago

It has always been in the old TMPE repo? :)

krzychu124 commented 5 years ago

Yup, but outdated currently

VictorPhilipp commented 5 years ago

That's the same branch where I started to implement the emergency evasion feature. I will update the code to the latest version of C:S.

pcfantasy commented 5 years ago

@krzychu124 @VictorPhilipp

Yes, I aslo start to use harmony in my mod, it is a fantastic lib for detour.

But I have a question, harmony is very useful for adding prefix and postfix in vanilla methods. But for some other vanilla methods, we have totally rewrite all the logics, is it better to use old cities-skylines-detour https://github.com/sschoener/cities-skylines-detour

VictorPhilipp commented 5 years ago

Harmony has its limits, I agree.

As you said it's good for prefixes and postfixes but transpiling (=replacing) methods with Harmony gives ton of problems:

originalfoo commented 5 years ago

I guess we just port to harmony where possible (as that will reduce conflict points with other mods and lower risk of problems with new game versions) and stick to detouring for the other stuff?

Strdate commented 5 years ago

Why do you need method transpiling? If you want to override the method completely, just use prefix and return false to indicate that the original method should not be run. Or I didn't get the point. I don't even know what transpiling is for, besides accessing original IL code, which you surely don't need in this case.

On Sun, 24 Mar 2019, 23:50 Aubergine, notifications@github.com wrote:

I guess we just port to harmony where possible (as that will reduce conflict points with other mods and lower risk of problems with new game versions) and stick to detouring for the other stuff?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/krzychu124/Cities-Skylines-Traffic-Manager-President-Edition/issues/119#issuecomment-476009308, or mute the thread https://github.com/notifications/unsubscribe-auth/AWNlulNEAf-EDpw7EIiwPbp5rezqNJfvks5vaAEpgaJpZM4bZzjw .

pcfantasy commented 5 years ago

@Strdate

No, if in your case, we do not need to use harmony. We`d like to use harmony to reduce conflict, that other modders can also do some patch on the same method.

VictorPhilipp commented 5 years ago

@Strdate Prefix methods cannot set the return value, transpilers can

Strdate commented 5 years ago

@VictorPhilipp Are you sure? Just obtain the result pointer with the ref object __result and you are done. If you don't run the original method the return value should persist. (I didn't test it myself)