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

Extract all logic in modifiedRunLoop to small injections #1770

Closed
wants to merge 7 commits into from

Conversation

altrisi
Copy link
Collaborator

@altrisi altrisi commented Jul 26, 2023

Reviewing wanted! I'm not good at all with mixins, and although this has taken me a while and I've tried to check and the output is similar, it may not be the same! And this is quite a core part of Carpet (and vanilla!) that I'd prefer to not have messed up.

This PR gets rid of modifiedRunLoop and instead separates it into smaller mixins to the specific places where Carpet was changing its behaviour. This has the following benefits:

  • Removes the possibility for it to get out of sync with vanilla (it actually already was, though only vs a JFR profiling event) and having to check for that
  • Improves mod compatibility with mods that change other stuff in the run loop. Currently any mod that injected any logic there would just be silently skipped, IMO the worst of compatibility issues
  • Removes it from logs in a few places where we wrapped vanilla logic reducing reports and confusion with people thinking the crashes were caused by Carpet
  • Removes the workaround of TimeProfiler being private that meant we had to copy all the logic that used it with our own type

As a compromise, the local variable msThisTick is now global, but shouldn't be an issue given that should always accessed by the same thread and in the same section.

Missing (only ideally, likely not really needed for it to just work):

  • Replacing l = Util.getMillis() - this.nextTickTime; with our l in the preTick (currently lost)

Resolves #1747.

@altrisi altrisi requested a review from gnembon July 26, 2023 13:31
@altrisi
Copy link
Collaborator Author

altrisi commented Jul 27, 2023

Currently crashes with g4mespeed.

@altrisi altrisi marked this pull request as draft July 29, 2023 18:08
@altrisi altrisi closed this Oct 25, 2023
@altrisi
Copy link
Collaborator Author

altrisi commented Oct 25, 2023

It got extracted further it seems!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor modifiedRunLoop mixin to not wrap vanilla logic
1 participant