AnnulusGames / LitMotion

Lightning-fast and Zero Allocation Tween Library for Unity.
https://annulusgames.github.io/LitMotion/
MIT License
841 stars 66 forks source link

Add: AnimationCurve support #110

Closed AnnulusGames closed 8 months ago

Akeit0 commented 8 months ago
  1. It would be better to return NativeAnimationCurve to application shared pool as much as possible. And returning to pool on MotionStorage.Reset is also needed. (I think you already know this.)
  2. NativeAnimationCurve should be named UnsafeAnimationCurve, and NativeAnimationCurve should be a pointer wrapper. (This allows to share memory with planned functionPointer.)
AnnulusGames commented 8 months ago

It would be better to return NativeAnimationCurve to application shared pool as much as possible. And returning to pool on MotionStorage.Reset is also needed. (I think you already know this.)

Yes, it's definitely better from a memory consumption perspective. However, returns to the pool are somewhat complex to incorporate into current implementations, and there is a risk of double returns. (This can be avoided by adding some checks, but it will reduce performance during creation.) In the current implementation, in the worst case, as many NativeAnimationCurves as there are elements in the MotionData array are created. However, this is rarely a problem in practice.

NativeAnimationCurve should be named UnsafeAnimationCurve, and NativeAnimationCurve should be a pointer wrapper. (This allows to share memory with planned functionPointer.)

I think FunctionPointer and NativeAnimationCurve should be kept in separate fields. In particular, the delegates generated by FuncitonPointer need to be cached if Burst is not used, and handling them with a single pointer complicates this process. The concern is that the structure size will increase, but this shouldn't be a problem since MotionData is originally a huge struct and was designed to avoid copying. (Also, from a safety perspective, I would like to avoid implementations that do not use AtomicSafetyHandle.)

AnnulusGames commented 8 months ago

My main concern with this implementation is the risk of memory leaks and double freeing. They have passed minimal testing and should not be a problem, but I am not too sure... (especially the crash caused by double deallocation, which must be avoided at all costs...)

Akeit0 commented 8 months ago

OK, but considering EnterPlayModeSettings, it is mandatory to dispose or pool with MotionStorage.Reset.

AnnulusGames commented 8 months ago

Yes, I think so. In that case, would it be better to have the Allocator on the MotionStorage side?

Akeit0 commented 8 months ago

As for naming, I think it is inevitable that NativeAnimationCurve will be UnsafeAnimationCurve if we follow Unity's naming rules. (like UnsafeList and NativeList)

AnnulusGames commented 8 months ago

I consider containers whose safety is checked by AtomicSafetyHandle to be Native-, and containers that are not checked to be Unsafe-. Considering this, wouldn't Native- be more appropriate in this case?

Akeit0 commented 8 months ago

Sorry, you seem to be right. I was mistaken, as the behavior of the field being overwritten by CopyFrom is not seen in Native-.

Akeit0 commented 8 months ago

Isn't it safe to release animationcurve in MotionStorage.RemoveAll? If this causes some issue, it means the current implementation of MotionStorage is incorrect.

AnnulusGames commented 8 months ago

I considered several implementations and made changes to the implementation.

  1. In the previous implementation, the safety check does not work for RewindableAllocator. Therefore, I changed NativeAnimationCurve to be a wrapper for NativeList. (I have already tested that this works correctly.)
  2. Changed SharedRewindableAllocator to generic, and now it is possible to share Allocator using type as key. Although this was not necessary for this implementation, it is a change that will allow us to reuse this type in the future.
  3. After investigating, I determined that there is no need to Dispose when resetting. Also, it is faster to allocate memory in advance using RewindableAllocator than to dispose at every remove.