Add ability to play animations on a separate instance #7919
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@mentholyspirit could sigh the CLA so we could submit the change eventually? |
mentholyspirit
force-pushed
the
feature/reusable-animation
branch
2 times, most recently
from
0975047
to
8f617eb
Compare
|
Signed the CLA |
There was a problem hiding this comment.
new ctor should not be inline and should be clearly documented.
lifetime management of the Animator on the java side seems sketchy to me, I'll defer to @romainguy here.
| public: | ||
| Animator(FilamentAsset *asset, FilamentInstance *instance) : Animator(reinterpret_cast<FFilamentAsset*>(asset), reinterpret_cast<FFilamentInstance*>(instance)) | ||
| { | ||
| } | ||
| ~Animator(); |
There was a problem hiding this comment.
move this to the existing public section and do not inline the implementation. You also need to document what this ctor does, how to use it and explain that in this case the caller must delete it. You also need to explain the expected lifetime of the asset and the instance with respect to the lifetime of the Animator.
Because this constructor will be confusing you must explain in the documentation that the normal way of getting an Animator is to call FilamentInstance::getAnimator() and that this new ctor is for a specific use case.
Documentation is everything here.
| @Override | ||
| public void finalize() { | ||
| try { | ||
| super.finalize(); | ||
| } catch (Throwable t) { // Ignore | ||
| } finally { | ||
| if (mIsOwner) { | ||
| nDestroyAnimator(mNativeObject); | ||
| mNativeObject = 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't like this -- I think we can't rely on finalizers from being called. I'm not sure what the correct pattern would be here. @romainguy what do you think?
There was a problem hiding this comment.
Finalizers are unfortunately not reliable (and at best take a lot of time to be invoked). We did use them in a couple of places but it's better to have a manual destroy().
mentholyspirit
force-pushed
the
feature/reusable-animation
branch
from
58fb7f2
to
a1be171
Compare
| @@ -37,11 +37,30 @@ | |||
| */ | |||
| public class Animator { | |||
| private long mNativeObject; | |||
| private Boolean mIsOwner = false; | |||
There was a problem hiding this comment.
This should be lower case boolean, otherwise it's an object.
| @Override | ||
| public void finalize() { | ||
| try { | ||
| super.finalize(); | ||
| } catch (Throwable t) { // Ignore | ||
| } finally { | ||
| if (mIsOwner) { | ||
| nDestroyAnimator(mNativeObject); | ||
| mNativeObject = 0; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Finalizers are unfortunately not reliable (and at best take a lot of time to be invoked). We did use them in a couple of places but it's better to have a manual destroy().
related to #7622