Fix spinner cheese by accounting for spin directionality

[peppy]

• Supersedes Rework spinners to track full rotations (part 2) #24999
• Closes Spinners can be cheesed #8224

I've simplified things in this PR. Hopefully I've left enough inline commenting to explain the reasoning behind things.

Have a read through this and see what you think. I'm up for explaining rationale over voice if it helps, but my summary thoughts are (mostly @smoogipoo as we have had countless of hours of discussion and both understand the caveats we encountered in previous implementations):

Without proper important frame accounting of spinners, this is as-good-as-or-better-than master/stable. Given two frames, the maximum angle we can accurately calculate is 180 degrees. So the goal going forward should be to, for the sake of replay accuracy, factor this in to important frames.

That is out of scope of this change, but is a consideration of this implementation. It simplifies things greatly in both code and thought process.

I have not given due diligence to tests. These are taken from #24999. The only changes/additions I made to tests are in 9011170 and 80ac0c4.

[bdach]

I think this is likely fine but I haven't put as much thought into it as you probably have with respect to how this might break...

[smoogipoo]

I don't think this can ever handle anything that happens within one "spin" (as I think you've termed it). I'm fairly certain rewinding is completely broken and irrecoverable with this implementation.

[smoogipoo]

The original test was correct - this should be 180 because we've rewound somewhere in the segment [1000, 2000). We haven't done (and aren't in the process of doing) the 450 rotation yet - that happens in one instant at t = 2000.

Suggested change

	history.ReportDelta(1750, -45);
	Assert.That(history.TotalRotation, Is.EqualTo(315));
	history.ReportDelta(1750, -45);
	Assert.That(history.TotalRotation, Is.EqualTo(180));

[peppy]

The change is intentional and correct to my eyes, but explaining this may require a voice call if it doesn't immediately make sense as to why. The important frame consideration in the PR description comes into play here.

[smoogipoo]

As discussed in voice, the primary issue here is that it should be impossible for the (1750, -45) report to exist as it's passing through multiple 180-degree turns, even without important frames. I still feel like this test is illogical in its current existence, and either needs to be updated (as I did - passing through 0 from a point very close to 0) or removed.

probably not reviewing this again since clearly i have no idea

[peppy]

I don't think this can ever handle anything that happens within one "spin" (as I think you've termed it). I'm fairly certain rewinding is completely broken and irrecoverable with this implementation.

Did you want to wait to discuss this IRL, or did you have a specific test showing where you think this is broken?

[smoogipoo]

As discussed, it may be good to have these assert (4000, 900) again after each rewind step, to ensure forward playback is still correct.

[peppy]

Have added this, fixed the clock interpolation and removed the test lenience as it is now precise \o/.

[smoogipoo]

I still find it exceedingly hard to follow through what the total rotation should be given a sequence of movements, without actually seeing the final value first in practice...

This still does not make sense to me: #25157 (comment)
As does this: #25157 (comment)

But the tests work sooooo... ./shrug?

I don't know if this is an algorithmic failing (especially since the above is a visual test, so it's respecting frame stability) - that is; the algorithm is working but giving extremely unintuitive values, or just more time needs to be spent understanding how the algorithm leads to this, or what.
Just noting it down but resolving issues as I don't know where to go from that.

[smoogipoo]

Looks like the test changes broke everything else :(

[peppy]

Weird, I ran a full test run too.

[smoogipoo]

The editor behaves weirdly around spinners - sometimes showing negative RPM values, bonus score starting at high points, etc. I don't believe that's ever going to be fixable by this, correct? We would either need to support exactly the case I was trying to fix before, or somehow enable frame stability around just spinners (which sounds like terrible UX for the editor).

[smoogipoo]

If this is to be merged, it is only on the basis that rewinding is a TBD/rudimentary implementation. I'm still not happy with it and I can guarantee that it's going to give us a lot of headaches in its current form - the editor as I've mentioned is one, but also potentially instant-retry as I've implemented in the past, and anything else that may rely on non-frame-stable playback.

[peppy]

Editor is an edge case which doesn't matter at this point. It was not correct before and is not correct now.

Also, spinners aren't the only thing broken by non-frame-stable playback. The editor needs frame stable playback to some degree. We need to make it efficient enough for this to be always-on, or need to (ab)use knowledge of alive objects / lifetimes to enable it only for nearby past/future.