Tracktion / tracktion_engine

Tracktion Engine module
Other
1.21k stars 152 forks source link

[Bug]: Clip::trimAwayOverlap does something even when there's no overlap #197

Closed chrhaase closed 5 months ago

chrhaase commented 8 months ago

Detailed steps on how to reproduce the bug

I'm using Clip::trimAwayOverlap for implementing a 'no overlap' behaviour when dragging clips. I would expect the function to not do anything if the input range doesn't overlap the clip's range, but this is not the case.

Would it be compatible with your usage of it to add a check like:

void Clip::trimAwayOverlap (TimeRange r)
{
    auto pos = getPosition();

    if (! pos.time.overlaps (r))
        return;

    if (r.getEnd() > pos.time.getStart())
    {
        if (r.getEnd() < pos.time.getEnd())
            setStart (r.getEnd(), true, false);
        else if (pos.time.getStart() < r.getStart())
            setEnd (r.getStart(), true);
    }
}

And maybe even:

if (r.contains (pos.time))
    removeFromParent();

I can of course add these checks before calling trimAwayOverlap if the current behaviour is actually intended!

What is the expected behaviour?

See above

Unit test to reproduce the error?

class TestClassName : public juce::UnitTest
{
public:
    TestClassName() : juce::UnitTest ("TestClass", "tracktion_engine") {}

    void runTest() override
    {
        beginTest ("Test section");
        {
            auto& engine = *tracktion::engine::Engine::getEngines()[0];
            auto edit = Edit::createSingleTrackEdit (engine);

            auto clip = getAudioTracks (*edit)[0]->insertMIDIClip ({ 0_tp, 4_tp }, nullptr);
            auto positionBeforeTrim = clip->getPosition();

            clip->trimAwayOverlap ({ 6_tp, 8_tp });

            expect (clip->getPosition().time == positionBeforeTrim.time);
        }
    }
};

// Creates an instance of the test so you can run it
static TestClassName testClassName;

// Call this from your applications
inline void runTest()
{
    juce::UnitTestRunner runner;
    runner.runTests ({ &testClassName });
}

Operating systems

Windows, macOS, Linux

What versions of the operating systems?

Not really OS dependent...

Architectures

x86_64

Stacktrace

No response

Plug-in formats (if applicable)

No response

Plug-in host applications (DAWs) (if applicable)

No response

Testing on the develop branch

The bug is present on the develop branch

Code of Conduct

drowaudio commented 5 months ago

Thanks for the test! Fixed here: https://github.com/Tracktion/tracktion_engine/commit/837f510d0856bf0b01391e78bfdeae727a876804