rive-cpp-legacy icon indicating copy to clipboard operation
rive-cpp-legacy copied to clipboard

Deal with LinearAnimationInstance with no backing Animation.

Open luigi-rosso opened this issue 3 years ago • 3 comments

At design time, a designer can create a Blend state which references multiple animations. If those animations values are not set (via a dropdown in the Blend State inspector) they will be exported with a "default/missing id".

I provide a fix here, but I strongly suspect this is the wrong approach as it bloats the code and makes it have to deal with these ugly conditions.

image

When the blend animation currently imports, we don't check if its id is equal to the "missing id": https://github.com/rive-app/rive-cpp/blob/a3f7e84568498ad99a24239987447b69e743becb/src/animation/blend_animation.cpp#L24-L26

What could do here is check if animationId() == -1 (or a helper isMissingId()) we could return an invalid status code like StatusCode::InvalidObject and fail the import. Something like:

    if(animationId() == -1) {
        return StatusCode::InvalidObject;
    }
    auto artboard = artboardImporter->artboard();
    size_t animationCount = artboard->animationCount();
    if ((size_t)animationId() < animationCount) {
        m_Animation = artboardImporter->artboard()->animation(animationId());
    }

Currently, this results in a nullptr m_Animation. So when the BlendState becomes active, it attempts to make an instance of the null animation. Our AnimationInstance class does not deal currently deal with a null backing animation. This PR adds that messy logic and fixes the issue by attempting to let the file limp along and play without applying the missing animation.

I think the right thing to do is (pinging @neurowave and @alxgibsn for these considerations):

  • Make it clearer in the editor when you've forgotten to assign an animation (maybe an alert icon on the BlendState itself in the graph or a warning at export?).
  • Drop any bad states at export, yes this will create a non-high fidelity file but it'll at least let you "export anyway" if you really want to and generate a valid file the runtime won't error on load with.
  • Runtime should error on load as described above if it finds a blend state referencing a null animation.

luigi-rosso avatar Apr 25 '22 18:04 luigi-rosso

+1 for aborting.

Rather than checking explicitly for -1... we could just check m_Animation at the end of that function. If it is still null, we're malformed and should abort.

mikerreed avatar Apr 25 '22 19:04 mikerreed

if ((size_t)animationId() < animationCount) { m_Animation = artboardImporter->artboard()->animation(animationId()); } if (!m_Animation) { return StatusCode::InvalidObject; }

mikerreed avatar Apr 25 '22 19:04 mikerreed

I'll let @mikerreed do the formal review, but this looks reasonable to me. Thanks for adding me -- reading PRs is helping me get up to speed on the rest of Rive!

csmartdalton avatar Apr 25 '22 20:04 csmartdalton