Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deal with LinearAnimationInstance with no backing Animation. #270

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

luigi-rosso
Copy link
Collaborator

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.

@mikerreed
Copy link
Contributor

+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
Copy link
Contributor

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

@csmartdalton
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants