Refactor `modifiedRunLoop` mixin to not wrap vanilla logic
altrisi opened this issue ยท 6 comments
It's incredibly common for people see carpet$modifiedRunLoop
in stack traces and blame Carpet. Some of these get into issues here, some in the discord, some in other discords (like Fabric's), where they'll be told Carpet looks to be the cause and should be removed.
That piece of code should be refactored so it isn't a block wrapping vanilla, but specific changes where it's needed, possibly also helping compatibility and preventing unwilling behaviour changes if that code changes and it gets unnoticed.
We can't do much of anything without ASM here can we?
I assume the problem is that @ModifyConstant can't change the type? But then I'm not sure I get that, as to remove the wrapper cleanly we would not want to do such a thing anyway.
I'm not very familiar with the tick rate/warp code, but from a look it seems we can just manage the float part ourselves, and modify the constants with a method to return the long cast version.
I'm also not very experienced with mixins in general of course, so maybe I am completely missing something vital.
Does seem like it should be doable though, I will try it out and see what happens.
This seems to work @altrisi, it is not entirely complete (doesn't handle the accumulator) and may be potentially problematic (short-circuit cancel of haveTime() may not be safe, need to investigate further).
But it works. (With very limited testing so far ofc).
I was about to reply that casting to long there would lead to precision issues. I had actually gotten pretty much that far but stopped after hitting the float block (was thinking maybe redirecting the whole store or something, but it started to get more complex).
Now I just checked and found out that the mspt
field is already always casted to a long before being assigned, and therefore this actually is no issue at all. Even pre-refactor of tick speed handling it was a long in disguise.
Now I wonder if it'd make sense to just store it as a long given that's how it's used, and also whether it'd make it more precise to store the actual decimal value of the division to make /tick rate
more precise (though then of course this'd be complicated again).
(re: initial questions: Yes, the problem I was facing was exactly that, ModifyConstant
from long
->float
, but I originally thought casting there would lose precision)
I guess I'll look into it again soon, possibly from your branch if that's not an issue given I think I deleted mine.
(for the locals that are used in multiple places I was thinking just storing them in fields in the mixin, given it should only ever be executed by one thread anyway so that shouldn't be an issue)
For the locals I only introduced one field, and because I couldn't figure out how to inject what is called determineTickSpeed()
separately and realised I can just call it from the ModifyVariable anyway, which is better, it can actually be removed too, as returning it from that ModifyVariable is the only place it is used.
Do need to fix up the accumulator thing in some way, discovered that code was actually introduced by authors of https://github.com/G4me4u/g4mespeed for compatibility, their mod does some interesting looking things client side (smoothing), so it could be effected some way by changing this.