Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

GameSA inline assembly cleanup #2649

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

Closed
Merlin wants to merge 82 commits into multitheftauto:master from Merlin:inline_assembly_cleanup

Conversation

@Merlin
Copy link
Contributor

@Merlin Merlin commented Jun 22, 2022

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.

AlexTMjugador reacted with heart emoji lopezloo, turret001, curukcesetgurmesi, theSarrum, AlexTMjugador, Proxy-99, and Bence58 reacted with rocket emoji
Merlin added 30 commits June 21, 2022 12:47
Copy link
Contributor

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.

Copy link
Member

sbx320 commented Jun 24, 2022

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.

AlexTMjugador, theSarrum, PlatinMTA, lopezloo, and MrGKanev reacted with thumbs up emoji

Copy link
Contributor Author

Merlin commented Jun 27, 2022

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?

Copy link
Member

sbx320 commented Jun 27, 2022

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

Merlin, lopezloo, and AlexTMjugador reacted with thumbs up emoji

Copy link
Member

@sbx320 sbx320 left a comment

Choose a reason for hiding this comment

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

Found a few mistakes and unclear changes. Otherwise it looks good.

Long-term we might want to move the types into the address declarations, but that's a separate task.

sbx320
sbx320 previously approved these changes Jul 8, 2022
Copy link
Contributor

Pirulax commented Aug 15, 2022

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!

Copy link
Contributor Author

Merlin commented Dec 29, 2022

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.

Copy link
Member

sbx320 commented Dec 29, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sbx320 sbx320 sbx320 approved these changes

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /