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

Migration to Harmony #119

Closed
krzychu124 opened this issue Mar 1, 2019 · 10 comments · Fixed by #260
Closed

Migration to Harmony #119

krzychu124 opened this issue Mar 1, 2019 · 10 comments · Fixed by #260
Assignees
Labels
code cleanup Refactor code, remove old code, improve maintainability discussion Contains debate on certain topics enhancement Improve existing feature technical Tasks that need to be performed in order to improve quality and maintainability under-review A pull request has been created and is currently being reviewed
Milestone

Comments

@krzychu124
Copy link
Member

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 😄 ) 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.

@krzychu124 krzychu124 added enhancement Improve existing feature discussion Contains debate on certain topics labels Mar 1, 2019
@krzychu124 krzychu124 added this to the 1.11.0 milestone Mar 1, 2019
@Strdate
Copy link

Strdate commented Mar 1, 2019

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

@krzychu124
Copy link
Member Author

krzychu124 commented Mar 1, 2019

Yup, but outdated currently

@VictorPhilipp
Copy link
Collaborator

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.

@VictorPhilipp VictorPhilipp added the technical Tasks that need to be performed in order to improve quality and maintainability label Mar 14, 2019
@VictorPhilipp VictorPhilipp self-assigned this Mar 14, 2019
VictorPhilipp added a commit that referenced this issue Mar 14, 2019
@VictorPhilipp VictorPhilipp added the in-progress The problem is being solved currently label Mar 17, 2019
VictorPhilipp added a commit that referenced this issue Mar 19, 2019
@pcfantasy
Copy link
Contributor

@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
Copy link
Collaborator

VictorPhilipp commented Mar 23, 2019

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:

  • Detouring struct methods seems impossible with Harmony and Harmony cannot handle methods that return structs (Incompatible method signature generation for instance methods returning structs pardeike/Harmony#77)
  • Calls to base class methods / private methods are impossible.
  • Annotation driven patching does not work when there are multiple candidate methods that receive struct references as arguments (you would need to put a typeof(TheStructType).MakeByRefType() which is not a constant expression).

@originalfoo
Copy link
Member

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
Copy link

Strdate commented Mar 26, 2019 via email

@pcfantasy
Copy link
Contributor

@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
Copy link
Collaborator

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

@Strdate
Copy link

Strdate commented Mar 29, 2019

@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)

VictorPhilipp added a commit that referenced this issue Apr 2, 2019
@VictorPhilipp VictorPhilipp added under-review A pull request has been created and is currently being reviewed and removed in-progress The problem is being solved currently labels Jun 6, 2019
@originalfoo originalfoo added the code cleanup Refactor code, remove old code, improve maintainability label Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code cleanup Refactor code, remove old code, improve maintainability discussion Contains debate on certain topics enhancement Improve existing feature technical Tasks that need to be performed in order to improve quality and maintainability under-review A pull request has been created and is currently being reviewed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants