Revert "Fix interaction between global offset and music rate."

[phantom10111]

This reverts commit 343bbd0.

Global offset is intended to compensate for input and audio latency in the PC used for playing. This latency stays constant regardless of music rate, so it's incorrect to multiply it. The original commit states that we need the multiply to arrive at the same position in the sound file regardless of music rate, but in fact we want the position to be different for different rates in order to compensate for the latency correctly.

[phantom10111]

This should fix Simply-Love/Simply-Love-SM5#557

[phantom10111]

Actually even though reverting the commit fixes the issue as described in the report (add 100s to song offset and -100 to global offset), I started getting second thoughts about if this is actually correct. I think I need to sleep on this a little bit and do some more testing of different cases.

[sukibaby]

Thank you very much for looking into this @phantom10111 .

Let's break down the commit you referenced:

In the original code, the GetBeatAndBPSFromElapsedTime function modifies the elapsed_time by adding PREFSMAN->m_fGlobalOffsetSeconds. Makes sense.

In Matt McCutchen's commit, the GetBeatAndBPSFromElapsedTime function adds the product of GAMESTATE->m_SongOptions.GetCurrent().m_fMusicRate and PREFSMAN->m_fGlobalOffsetSeconds. This is wrong because it doesn't reconcile the differences in the different time formats correctly.

In Matt McCutchen's commit, the GetElapsedTimeFromBeat function also incorrectly modifies the global offset value.

I dug up the PR that this came from, and I am not pleased with what I saw. My biggest issue with it is that it was known to break "haste" mode but they committed it anyway and said deal with it later if it breaks stuff. So I guess we are fixing it now. stepmania/stepmania#828

edit: happy ITGm issue 300 !

[sukibaby]

Instead of directly adding the global offset to elapsed_time, we should convert the global offset to the sound file offset. Then, we should apply the adjusted offset to elapsed_time.

[sukibaby]

We should also convert the global offset to the sound file offset here, then subtract the adjusted offset from the result.

[phantom10111]

After some testing I can definitely say that this revert isn't correct. I did some tests with introducing some offset at song level and then I could only get correct results with the revert applied. But I also did some tests with introducing offset at engine level (basically emulating severe audio lag) and then I could only get correct results without the revert.

@sukibaby I'm not sure I understand your comment about converting the global offset to sound-file offset. Isn't multiplying by music rate doing exactly that? Purely from the point of maths, I don't have any other idea how to do it, either we multiply (and apply the offset in wall-clock time domain) or we skip the multiply (and apply the offset in music time domain).

But I gave this issue a bit more thought and I don't think we can actually fix this easily (unless I'm mistaken about the math). But right now I'm thinking this - in the ideal case the song should be synced to 0ms and global offset only covers the audio and input lag. In that case we do the multiply and apply the offset in wall-clock time domain, which is what the code does currently. But in 9ms case the song is synced to 9ms and then global offset needs to account for audio and input lag + the 9ms. But the problem is that audio and input lag are both in wall-clock time domain (and therefore need the multiply), whereas the 9ms is in the song domain (and shouldn't be multiplied). So I right now I don't have any idea other than doing weird things like splitting global offset into 2 separate settings, or waiting until something like pack.ini is implemented and using information from that.

If I'm mistaken and we actually have all the information required to calculate this I'll be happy to hear it but right now I'm out of ideas.

[sukibaby]

@sukibaby I'm not sure I understand your comment about converting the global offset to sound-file offset. Isn't multiplying by music rate doing exactly that? Purely from the point of maths, I don't have any other idea how to do it, either we multiply (and apply the offset in wall-clock time domain) or we skip the multiply (and apply the offset in music time domain).

I think the problem is that beats and time in seconds are being mixed, which aren't the same time factor.

So for GetElapsedTimeFromBeat, we should be able to

• divide the global offset by the duration of a single beat, store it as OffsetInBeats
• store TimingData::GetElapsedTimeFromBeatNoOffset(fBeat) as ElapsedTime
• multiply m_fMusicRate by OffsetInBeats, store it as AdjustedOffset
• return ElapsedTime - AdjustedOffset

we could also optionally break early if m_fMusicRate == 1.

[Fieoner]

regarding Simply-Love/Simply-Love-SM5#557 @phantom10111 you are correct in that we don't have all the information required to solve the issue for 9ms biased simfiles yet. With pack.ini support it will be possible to do as you said and split the offset in 2 parts and adjust them accordingly.

[sukibaby]

pack.ini will be a life saver when it is implemented, but there will still be a very large amount of content which never gets updated with a new pack.ini. We should try to mitigate the problem as much as possible for that situation.

It seems to me that we need to be able to accurately determine the elapsed time and current BPM based on a given row, or we need to be able to determine the current beat.

We can do something like this...

float TimingData::GetCurrentBPM()
{
    // Get the current beat
    float currentBeat = GAMESTATE->m_Position.m_fSongBeat;

    // Convert beat to row
    int currentNoteRow = BeatToNoteRow(currentBeat);
   
    // Retrieve the BPM segment for the current row
    const TimingSegment* segment = GetSegmentAtRow(currentNoteRow, SEGMENT_BPM);

    // Extract the BPM value
    if (segment != DummySegments[SEGMENT_BPM])
    {
        const BPMSegment* bpmSegment = static_cast<const BPMSegment*>(segment);
        if (bpmSegment)
        {
            return bpmSegment->GetBPM();
        }
    }
}

float TimingData::GetElapsedTimeFromBeat(float fBeat) const
{
	// Get the BPM at the calculated row
	float bpm = GetCurrentBPM();

	// Convert BPM to BPS
	float bps = bpm / 60.0f;

	// Convert the global offset from seconds to beats
	float globalOffsetInBeats = PREFSMAN->m_fGlobalOffsetSeconds * bps;

	// Adjust the calculation to use globalOffsetInBeats
	// Note: We need to convert globalOffsetInBeats back to seconds to adjust the elapsed time correctly
	// This involves dividing by bps (since globalOffsetInBeats * (1/bps) = globalOffsetInSeconds)
	float elapsedTime = TimingData::GetElapsedTimeFromBeatNoOffset(fBeat)
		- (globalOffsetInBeats / bps) * GAMESTATE->m_SongOptions.GetCurrent().m_fMusicRate;

	return elapsedTime;
}

void TimingData::GetBeatAndBPSFromElapsedTime(GetBeatArgs& args) const
{ 
	// Get the BPM at the calculated row
	float bpm = GetCurrentBPM();

	// Convert BPM to BPS
	float bps = bpm / 60.0f;

	// Convert the global offset from seconds to beats
	float globalOffsetInBeats = PREFSMAN->m_fGlobalOffsetSeconds * bps;

	args.elapsed_time += globalOffsetInBeats / GAMESTATE->m_SongOptions.GetCurrent().m_fMusicRate;

	GetBeatAndBPSFromElapsedTimeNoOffset(args);
}

...and that should help us maintain greater accuracy than either keeping the current code or reverting to the old code.

That code above needs to be tested better, especially by someone with a better sense of timing than I have, but it does build, so hopefully it works.