mtasa-blue
mtasa-blue copied to clipboard
multitheftauto
GameSA inline assembly cleanup
Removes most of the inline assembly for GameSA. Only hooks and some function calls that are patched or unused are left.
I looked over all the files. But I may have messed up somewhere and didn't spot it.
Next thing I would like to do is cleaning up all the includes. And move all the *SAInterface classes to their own files and remove unused classes like CFont and a bunch of unused methods.
But before I do any of that it would be nice to know if these changes would be merged.
Any changes this big should be made in small chunks that can be reviewed by anyone, merged into a branch one by one, and then the branch can be merged into master when complete.
Now there is a lot to review at once.
We can't risk "I might have missed something" in these large pull requests.
It doesn't really matter too much whether this is a single PR with many commits or many PRs with many commits. As the changes are very localized (one isolated change per function), it's actually fairly easy to review in bulk. Just a lot of work to do.
I've thus far reviewed it up to CAutomobileSA.h and it looks good, might get to some more over the weekend. Merging however will be a bit more annoying, since we're unlikely to be able to test it entirely before that. Guess we'll just have to time it with a stable build and risk a broken nightly.
Sorry, I thought it would be possible to review and approve single commits from one PR till all are approved and then merge it. Never done any code reviews on github. Should i still split it? And if, how would I do that without creating a new branch and PR?
There really isn't a great way to do large scale PRs via Github. You'd need to create a PR for each commit for us to approve & merge individually. Doesn't really scale for this amount of commits though.
However we can just keep this in here and merge manually via git itself if we need to partially merge.
Generally I'm tending towards just trying to get some kind of nightly test build going with these changes merged (not the "default" nightly build though). Most issues that would be caused by these changes should result in immediate crashes, so it would be good to try it out on some common servers.
Anyway, 50/76 files reviewed thus far. Can't submit it until I'm actually done though...
plugin-sdk has a few functions meant for this exact purpose, those could've been used (CallMethod, etc..), it would've looked nicer, but still, good job!
I think it will be easier if I slowly introduce these calls with further game sa cleanups. Maybe with cleaned up game classes in separate files then their mirror/wrapper classes. That way we can also include them in Deathmatch/MultiplayerSA for quick debugging without introducing a bunch of virtual functions in mirror/wrapper classes. None of your review work would go to waste. I would just copy them into clean new classes.
Yeah, resolving those conflicts might be painful.
On a related note: I've been looking at in the past was to use some custom DSL tooling to automate manually writing these headers/calls. Essentially you'd create a file
class CVehicle
{
void BlowUp(bool bExplode) @ 0x123456;
}
and it could generate the corresponding C++ code out of it. My initial work on that was using Xtext, but Java is annoying so I didn't get too far. Might be worth looking into again.