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

Add: AnimationCurve support #110

Merged
merged 15 commits into from
Mar 18, 2024
Merged

Conversation

AnnulusGames
Copy link
Owner

No description provided.

@AnnulusGames AnnulusGames marked this pull request as ready for review March 18, 2024 03:21
@AnnulusGames AnnulusGames marked this pull request as draft March 18, 2024 03:21
@Akeit0
Copy link
Contributor

Akeit0 commented Mar 18, 2024

  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
Copy link
Owner Author

AnnulusGames commented Mar 18, 2024

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
Copy link
Owner Author

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

Akeit0 commented Mar 18, 2024

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

@AnnulusGames
Copy link
Owner Author

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

@Akeit0
Copy link
Contributor

Akeit0 commented Mar 18, 2024

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
Copy link
Owner Author

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

Akeit0 commented Mar 18, 2024

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

Akeit0 commented Mar 18, 2024

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
Copy link
Owner Author

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.

@AnnulusGames AnnulusGames marked this pull request as ready for review March 18, 2024 05:59
@AnnulusGames AnnulusGames merged commit e9f6e34 into main Mar 18, 2024
@AnnulusGames AnnulusGames deleted the feature-animationcurve-support branch March 18, 2024 09:42
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.

2 participants