feat: Animation of properties of LocalTransform #453

Rhuagh · Oct 25, 2017

Opening early to get feedback. Note that this PR only contains animation of properties on LocalTransform, and have no skinning support. That will come in a later PR.

Work left:

~~Testing!!!~~
~~Documentation~~
~~Interpolation needs a rework~~

Rhuagh · Oct 25, 2017

Rhuagh reviewed Oct 25, 2017

View changes

Rhuagh · Oct 25, 2017

amethyst_animation/src/resources.rs

+}
+
+/// Defines a single animation.
+/// Defines relationships between the node index in `AnimationHierarchy` and a `Sampler` in


Outdated docs.

Rhuagh · Oct 25, 2017

amethyst_animation/src/resources.rs

+    Done,
+}
+
+/// The actual animation data for a single attribute


Outdated docs.

Rhuagh · Oct 25, 2017

amethyst_animation/src/resources.rs

+/// Attaches to an entity, to control what animations are currently active
+#[derive(Clone)]
+pub struct AnimationControl {
+    /// Animation index into `AnimationStorage`


Outdated docs.

torkleyy · Oct 25, 2017

torkleyy reviewed Oct 25, 2017

View changes

A few first comments^^

torkleyy · Oct 25, 2017

Cargo.toml

@@ -42,6 +42,7 @@ thread_profiler = { version = "0.1", optional = true }
 [dev-dependencies]
 cgmath = "0.14"
 genmesh = "0.4"
+amethyst_animation = { path = "amethyst_animation", version = "0.1.0" }


So you decided not to reexport this package? Should we maybe do the same for the gltf crate then, which also doesn't seem like a "mandatory" crate?

Yeah, we should.

torkleyy · Oct 25, 2017

amethyst_animation/Cargo.toml

+repository = "https://github.com/amethyst/amethyst"
+
+readme = "README.md"
+license = "MIT OR Apache-2.0"


should be MIT/Apache-2.0

torkleyy · Oct 25, 2017

amethyst_animation/src/bundle.rs

+    }
+}
+
+/// Bundle for a complete animation setup including sampler interpolation and animation control


Should add comment that this automatically adds a SamplingBundle

torkleyy · Oct 25, 2017

amethyst_animation/src/interpolation.rs

+}
+
+pub trait Interpolate: Debug + Send + Sync {
+    fn interpolate_vec(


Are you planning on replacing this?

torkleyy · Oct 25, 2017

amethyst_animation/src/interpolation.rs

+    }
+}
+
+fn linear<D>(time_index: usize, time: f32, times: &Vec<f32>, values: &Vec<D>, normalize: bool) -> D


Still gotta check these functions

torkleyy · Oct 25, 2017

amethyst_animation/src/systems/control.rs

+
+
+fn do_control_set_pause(control_set: &mut SamplerControlSet) {
+    match control_set.translation {


Same as above

torkleyy · Oct 25, 2017

amethyst_animation/src/systems/control.rs

+}
+
+fn do_control_set_unpause(now: &Instant, control_set: &mut SamplerControlSet) {
+    match control_set.translation {


Same as above

torkleyy · Oct 25, 2017

amethyst_animation/src/systems/sampling.rs

+    fn run(&mut self, (samplers, mut controls, mut transforms): Self::SystemData) {
+        let now = Instant::now();
+        for (control_set, transform) in (&mut controls, &mut transforms).join() {
+            match control_set.translation {


could be if let Some(sampler) = control_set.translation.as_mut().and_then(|c| samplers.get(c.sampler))

torkleyy · Oct 25, 2017

amethyst_animation/src/systems/sampling.rs

+            do_sampling(&sampler, &duration, transform);
+        }
+        Done => do_end_control(&control, transform),
+        _ => (),


The idiomatic version would be _ => {} I think

torkleyy · Oct 25, 2017

amethyst_animation/src/systems/sampling.rs

+            RestState::Scale(s) => transform.scale = s,
+        },
+        // looping is handled during duration update
+        _ => (),


Same as above

After minterpolate has been released this will be ready. Need to get more testing done on that first though.

Rhuagh · Oct 26, 2017

After minterpolate has been released this will be ready. Need to get more testing done on that first though.

Ready for review.

Rhuagh · Oct 27, 2017

Ready for review.

torkleyy · Oct 28, 2017

torkleyy reviewed Oct 28, 2017

View changes

torkleyy · Oct 28, 2017

amethyst_animation/src/systems/control.rs

+/// - `sampler_storage`: `AssetStorage` for all `Sampler`s
+/// - `samplers`: the active sampler sets
+/// - `transforms`: `LocalTransform`s, used to retrieve the rest pose before animation starts.
+/// - `remove`: all entities pushed here will have the control object removed at the end of the system execution


~~If that's the case, could you use .drain() on the control storage?~~

Ah with "here" you mean pushed to the vector.

Btw, I love how nicely documented this is 👍

torkleyy · Oct 28, 2017

amethyst_animation/src/systems/control.rs

+        // and remove control object if all samplers are done and removed
+        (_, &AnimationCommand::Abort) | (&ControlState::Abort, _) | (&ControlState::Done, _) => {
+            if check_and_terminate_animation(hierarchy, samplers) {
+                remove.push(entity.clone());


🤔 This looks like a conditional push, so maybe that doc comment above is inaccurate?

torkleyy · Oct 28, 2017

amethyst_animation/src/util.rs

+        None => false,
+    } {
+        controls.insert(
+            entity.clone(),


Entities are Copy, no need to clone()

torkleyy · Oct 28, 2017

amethyst_animation/src/util.rs

+    entity: &Entity,
+) {
+    if let Some(ref mut control) = controls.get_mut(*entity) {
+        if let ControlState::Running(..) = control.state {


Maybe it would be convenient to have is_running and is_paused as methods?

torkleyy · Oct 28, 2017

amethyst_animation/src/util.rs

+pub fn toggle_animation(
+    controls: &mut WriteStorage<AnimationControl>,
+    animation: &Handle<Animation>,
+    entity: &Entity,


And since they're Copy, you don't need a reference.

torkleyy · Oct 28, 2017

amethyst_animation/src/util.rs

+        }) => true,
+        _ => false,
+    };
+    if running {


If you add those methods above, this could be

if controls.get(entity).map(|c| c.state.is_running()).unwrap_or(false) {}

torkleyy · Oct 28, 2017

torkleyy approved these changes Oct 28, 2017

View changes

Thanks for addressing, got a couple of nits but otherwise it's great

torkleyy · Oct 28, 2017

amethyst_animation/src/resources.rs

@@ -54,6 +55,7 @@ impl Component for AnimationHierarchy {
 
 /// Defines a single animation.
 /// Defines relationships between the node index in `AnimationHierarchy` and a `Sampler` handle.
+/// If the animation only target a single node index, `AnimationHierarchy` is not required.


should be "targets" I think

torkleyy · Oct 28, 2017

amethyst_animation/src/systems/control.rs

@@ -11,7 +11,8 @@ use resources::{Animation, AnimationCommand, AnimationControl, AnimationHierarch
 /// System for setting up animations, should run before `SamplerInterpolationSystem`.
 ///
 /// Will process all active `AnimationControl` + `AnimationHierarchy`, and do processing of the
-/// animations they describe.
+/// animations they describe. If an animation only target a single node/entity, there is no need for


torkleyy · Oct 28, 2017

amethyst_animation/src/systems/control.rs

+/// Check if the given animation list is for a single node. If so, we don't need an
+/// `AnimationHierarchy`.
+fn only_one_index(nodes: &Vec<(usize, Handle<Sampler>)>) -> bool {
+    if nodes.len() == 0 {


nit: should be nodes.is_empty()

torkleyy · Oct 28, 2017

amethyst_animation/src/systems/control.rs

@@ -70,14 +71,36 @@ impl<'a> System<'a> for AnimationControlSystem {
     }
 }
 
+/// Check if the given animation list is for a single node. If so, we don't need an
+/// `AnimationHierarchy`.
+fn only_one_index(nodes: &Vec<(usize, Handle<Sampler>)>) -> bool {


Can take slice here

torkleyy · Oct 28, 2017

torkleyy approved these changes Oct 28, 2017

View changes

I'll squash after approvals are in place.

Rhuagh · Oct 28, 2017

I'll squash after approvals are in place.

r? @Xaeroxe @Aceeri

Rhuagh · Oct 28, 2017

r? @Xaeroxe @Aceeri

omni-viral · Oct 29, 2017

omni-viral requested changes Oct 29, 2017

View changes

I like it. We really need good animation system.
I have a few questions.
Does this system supports

blending of animations
control animation speed
animations stacking
?

omni-viral · Oct 29, 2017

amethyst_animation/src/resources.rs

+#[derive(Clone, Debug)]
+pub struct Animation {
+    /// node index -> sampler handle
+    pub nodes: Vec<(usize, Handle<Sampler>)>,


Here should be Entity instead of usize.
Mappings such as in AnimationHierarchy should be covered by specs saveload feature.
But we need to develop PrefabMarker first.

No, because you can play animation on many different entities when you use prefabs.

You want to be able to share animations between many families of entities.

Can you describe use case? Which can't be done with attaching other entities to entities of prefab.

A quite common scenario for indie game development is that you have a single bone structure for many different units (think human, vulkan, klingon etc), and all units that share bone structure can share animations. This saves on time and cost of developing the game, because you don't have to produce as many animations.

So, animations are loaded once and then potentially shared between many different types of units/entities whatever.

The second issue with the suggestion you have is one of forward lookup. Given an animation and entity to play that animation on, how do i find which child entities to add samplers to ? If the only link we have is that the child entities have a reference to the entity of the prefab they were created from, you would have to look through the entire Parent (or some other component) storage to find the entitites that should be animated, which would be far to expensive imho. The other solution would be to have the AnimationHierarchy contain Entity -> Entity mappings. I went with the "node index -> Entity" solution, because it makes animation asset loading much, much easier.

Animations can also target only a part of a hierarchy, not necessarily the entire hierarchy. Example: Sword swing will probably only affect arms and upper torso.

And it would force anyone that wants to do animation on something to go via the prefab system, which might be unwanted. Note that this animation system has wider support than just skeletal animation, it can do animation of any LocalTransform, as long as the Animation can target it via AnimationHierarchy.

Regardless, this is not something that can be done within the scope of this PR, because it needs prefab loading first.

Example: Sword swing will probably only affect arms and upper torso.

But this system animate LocalTransform of entities. Will it share some parts with skeletal animation system?

Skeletal animation is this system together with vertex skinning.

omni-viral · Oct 29, 2017

amethyst_animation/src/resources.rs

+/// Sampler control set, containing optional samplers for each of the possible channels.
+///
+/// Note that the `AnimationOutput` in the `Sampler` referenced, and the `RestState` referenced in
+/// each `SamplerControl` should match the attribute channel from the set here. There is no check


Shouldn't be SamplerControl parametrised in this case?

This is in part due to how it looked at the start. I did consider changing this note, but came to the conclusion I wanted to get something out first. I will deal with this in a later PR.

omni-viral · Oct 29, 2017

amethyst_animation/src/systems/control.rs

+        let mut remove = Vec::default();
+        for (entity, control) in (&*entities, &mut controls).join() {
+            if let Some(state) = animation_storage.get(&control.animation).and_then(
+                |ref animation| {


What the point of ref keyword here?

Remnant of old code

omni-viral · Oct 29, 2017

amethyst_animation/src/systems/sampling.rs

+                        match times {
+                            // finite amount of looping, if 0 go do done,
+                            // else reduce iteration count and keep sampling
+                            Some(i) => if i <= 1 {


Use guards to avoid too many nesting levels

https://rustbyexample.com/flow_control/match/guard.html

This is a first version, so no blending, speed cobtrol or stacking no.

Rhuagh · Oct 29, 2017

This is a first version, so no blending, speed cobtrol or stacking no.

@Rhuagh ok. But you need to keep those features in mind so that adding them won't require total rewrite.

omni-viral · Oct 29, 2017

@Rhuagh ok. But you need to keep those features in mind so that adding them won't require total rewrite.

For the most part it will go something like this:

Speed control: additional parameter on AnimationCommand, and on SamplerControl.
Animation blending: SamplerControlSet gets a weight parameter, and it becomes Many<SamplerControlSet> as Component, Many<AnimationControl> as Component. Needs support in both systems.
Animation stacking: Basically the same as blending with the addition AnimationControl gets a control type parameter to differentiate between blend and stacking, and animation control system needs support for determining what to do at animation end.

Rhuagh · Oct 29, 2017

For the most part it will go something like this:

Speed control: additional parameter on AnimationCommand, and on SamplerControl.
Animation blending: SamplerControlSet gets a weight parameter, and it becomes Many<SamplerControlSet> as Component, Many<AnimationControl> as Component. Needs support in both systems.
Animation stacking: Basically the same as blending with the addition AnimationControl gets a control type parameter to differentiate between blend and stacking, and animation control system needs support for determining what to do at animation end.

@omni-viral review addressed and issues fixed, please review again.

Rhuagh · Oct 29, 2017

@omni-viral review addressed and issues fixed, please review again.

omni-viral · Oct 30, 2017

omni-viral requested changes Oct 30, 2017 • edited

View changes

A few more questions and suggestions.

omni-viral · Oct 30, 2017

amethyst_animation/src/systems/sampling.rs

+    );
+
+    fn run(&mut self, (samplers, mut controls, mut transforms): Self::SystemData) {
+        let now = Instant::now();


I believe that all systems should use central timer from amethyst_core

omni-viral · Oct 30, 2017

amethyst_animation/src/resources.rs

+    type HandleStorage = DenseVecStorage<Handle<Self>>;
+}
+
+impl Into<Result<Animation>> for Animation {


What for? Can't you use Ok function where you need to convert?

For asset processor.

You've answered only first question.

I'm using the Processor from the assets crate to do asset processing for Sampler and Animation, and it has Asset::Data = Animation (it converts into itself since it doesn't need a factory to convert or such), and that requires an impl Into<assets::Result<Asset>> for Asset::Data

Yes, I could do manual processing of them in my own system, just felt silly to build a new system for something that already exists.

omni-viral · Oct 30, 2017

amethyst_animation/src/systems/sampling.rs

+    }
+}
+
+fn next_duration(last_frame: f32, mut dur_s: f32) -> Duration {


It should return number of loops eaten by loop. This number should be subtracted from number of loops here

omni-viral · Oct 30, 2017

amethyst_animation/src/systems/sampling.rs

+                    ),
+                    EndControl::Loop(Some(i)) => (
+                        Running(now.clone(), next_duration(last_frame, duration_secs)),
+                        Some(EndControl::Loop(Some(i - 1))),


You should subtract number equal to time_elapsed / animation_duration and put time_elapsed % animation_duration into Running(_, >here<)

isn't modulo on floats kind of frowned upon?

omni-viral · Oct 30, 2017

amethyst_animation/src/systems/sampling.rs

+
+fn next_duration(last_frame: f32, mut dur_s: f32) -> Duration {
+    while last_frame != 0. && dur_s > last_frame {
+        dur_s -= last_frame;


Division should work faster then loop.
And you probably want to work with milliseconds in u32 (or nanoseconds in u64) instead of seconds in f32.

omni-viral · Oct 30, 2017

amethyst_gltf/src/format/mod.rs

+                },
+            ))
+        }
+        Weights => Err(GltfError::NotImplemented),


What are those? Skinning weights?

Morph target weights.

omni-viral · Oct 30, 2017

amethyst_animation/src/util.rs

+) {
+    if !match controls.get_mut(entity) {
+        Some(ref mut control) => {
+            if control.state.is_paused() {


If pause was requested then it will be paused on next system run.
Command should be set to Start unconditionally.

omni-viral · Oct 30, 2017

amethyst_animation/src/util.rs

+    end: EndControl,
+) {
+    if !match controls.get_mut(entity) {
+        Some(ref mut control) => {


Should check if it is the same animation.

omni-viral · Oct 30, 2017

amethyst_animation/src/util.rs

+    entity: Entity,
+    end: EndControl,
+) {
+    if !match controls.get_mut(entity) {


if !match is poorly readable.
Use single match block with guards or if let Some if appropriate.

The problem is borrowing rules. I'd have to break it up into two if let ..

omni-viral · Oct 30, 2017

amethyst_animation/src/util.rs

+        .map(|c| c.state.is_running())
+        .unwrap_or(false)
+    {
+        pause_animation(controls, entity);


What if another animation is running?

Since we only support a single animation atm, it felt like an unnecessary check for now.

Xaeroxe · Oct 30, 2017

Xaeroxe approved these changes Oct 30, 2017

View changes

Due to my inexperience with 3D animation it's really hard for me to say whether this is adequate or not, however I don't see anything here that particularly upsets me so I'll give this an approval with the disclaimer that I don't know a ton about what I'm reading here.

@omni-viral review comments addressed, please review again.

Rhuagh · Oct 30, 2017

@omni-viral review comments addressed, please review again.

Some more reviews on this would be really appreciated. I'll try to do another in-depth review on this, but one or two more wouldn't hurt.

torkleyy · Nov 1, 2017

Some more reviews on this would be really appreciated. I'll try to do another in-depth review on this, but one or two more wouldn't hurt.

omni-viral · Nov 1, 2017

omni-viral approved these changes Nov 1, 2017

View changes

bors r+

Xaeroxe · Nov 1, 2017

bors r+

Build succeeded

bors · Nov 1, 2017

Build succeeded

Rhuagh added status: working type: feature labels Oct 25, 2017

torkleyy added the project: animation label Oct 26, 2017

Rhuagh changed the title from [WIP] feat: Animation of properties of LocalTransform to feat: Animation of properties of LocalTransform Oct 26, 2017

Rhuagh added status: ready status: working and removed status: working status: ready labels Oct 26, 2017

Rhuagh added status: ready status: working and removed status: working status: ready labels Oct 28, 2017

Rhuagh added status: ready and removed status: working labels Oct 28, 2017

torkleyy requested a review from amethyst/engine-devs Oct 29, 2017

Rhuagh added some commits Oct 30, 2017

Add animation of properties of LocalTransform

27bb33f

Moving animation example

Loading status checks…

a41d4fa

bors bot merged commit a41d4fa into amethyst:develop Nov 1, 2017
2 of 3 checks passed

2 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details

bors Build succeeded

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Rhuagh deleted the Rhuagh:animation branch Nov 1, 2017

Aceeri added this to the 0.6.0 milestone Nov 28, 2017

amethyst/amethyst

Join GitHub today

feat: Animation of properties of LocalTransform #453

Conversation

Rhuagh commented Oct 25, 2017 • edited Edited 4 times Rhuagh edited Oct 26, 2017 (most recent) Rhuagh edited Oct 25, 2017 Rhuagh edited Oct 25, 2017 Rhuagh edited Oct 25, 2017

Rhuagh added status: working type: feature labels Oct 25, 2017

Rhuagh reviewed Oct 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy reviewed Oct 25, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy added the project: animation label Oct 26, 2017

Rhuagh changed the title from [WIP] feat: Animation of properties of LocalTransform to feat: Animation of properties of LocalTransform Oct 26, 2017

Rhuagh added status: ready status: working and removed status: working status: ready labels Oct 26, 2017

This comment has been minimized.

Rhuagh commented Oct 26, 2017

This comment has been minimized.

Rhuagh commented Oct 27, 2017

Rhuagh added status: ready status: working and removed status: working status: ready labels Oct 28, 2017

torkleyy reviewed Oct 28, 2017 View changes

This comment has been minimized.

torkleyy Oct 28, 2017 • edited Edited 2 times torkleyy edited Oct 28, 2017 (most recent) torkleyy edited Oct 28, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Rhuagh added status: ready and removed status: working labels Oct 28, 2017

torkleyy approved these changes Oct 28, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

torkleyy approved these changes Oct 28, 2017 View changes

This comment has been minimized.

Rhuagh commented Oct 28, 2017

This comment has been minimized.

Rhuagh commented Oct 28, 2017

torkleyy requested a review from amethyst/engine-devs Oct 29, 2017

omni-viral requested changes Oct 29, 2017 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

omni-viral Oct 29, 2017 • edited Edited 1 time omni-viral edited Oct 29, 2017 (most recent)

This comment has been minimized.

Rhuagh Oct 29, 2017 • edited Edited 1 time Rhuagh edited Oct 29, 2017 (most recent)

This comment has been minimized.

Rhuagh Oct 29, 2017 • edited Edited 2 times Rhuagh edited Oct 29, 2017 (most recent) Rhuagh edited Oct 29, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Rhuagh commented Oct 25, 2017 •

edited

Edited 4 times

Rhuagh edited Oct 26, 2017 (most recent)

Rhuagh edited Oct 25, 2017

Rhuagh edited Oct 25, 2017

Rhuagh edited Oct 25, 2017

Rhuagh reviewed Oct 25, 2017

View changes

torkleyy reviewed Oct 25, 2017

View changes

torkleyy reviewed Oct 28, 2017

View changes

torkleyy Oct 28, 2017 •

edited

Edited 2 times

torkleyy edited Oct 28, 2017 (most recent)

torkleyy edited Oct 28, 2017

torkleyy approved these changes Oct 28, 2017

View changes

torkleyy approved these changes Oct 28, 2017

View changes

omni-viral requested changes Oct 29, 2017

View changes

omni-viral Oct 29, 2017 •

edited

Edited 1 time

omni-viral edited Oct 29, 2017 (most recent)

Rhuagh Oct 29, 2017 •

edited

Edited 1 time

Rhuagh edited Oct 29, 2017 (most recent)

Rhuagh Oct 29, 2017 •

edited

Edited 2 times

Rhuagh edited Oct 29, 2017 (most recent)

Rhuagh edited Oct 29, 2017