feat: Generic animation system #558

Rhuagh · Feb 1, 2018

Taking a stab at making animation generic.

This change is

Ready for review.

Rhuagh · Feb 2, 2018

Ready for review.

This is great :)

Reviewed 19 of 19 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 16 at r1 (raw file):

              SamplerProcessor};

const DEP: [&str; 0] = [];

Why are you using a constant here? Isn't a reference to a ZST 'static automatically?

amethyst_animation/src/bundle.rs, line 59 at r1 (raw file):

/// Bundle for only the sampler interpolation.
///
/// Will add `SamplerInterpolationSystem` with name `sampler_interpolation_system`.

sampler_processor isn't listed here

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

        builder: DispatcherBuilder<'a, 'b>,
    ) -> Result<DispatcherBuilder<'a, 'b>> {
        if !world

I think we have added an Entry API for this.

amethyst_animation/src/resources.rs, line 15 at r1 (raw file):

pub trait AnimationSampling {
    /// The channel type
    type Channel;

Maybe add the Send + Sync + 'static bounds here?

amethyst_animation/src/resources.rs, line 23 at r1 (raw file):

    /// Get the current sample for a channel
    fn get_current_sample(&self, channel: &Self::Channel) -> SamplerPrimitive<Self::Scalar>;

Should be current_sample

amethyst_animation/src/resources.rs, line 264 at r1 (raw file):

        command: AnimationCommand,
    ) -> Self {
        Self {

Please use AnimationControl for the construction

Comments from Reviewable

torkleyy · Feb 3, 2018

This is great :)

Reviewed 19 of 19 files at r1.
Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 16 at r1 (raw file):

              SamplerProcessor};

const DEP: [&str; 0] = [];

Why are you using a constant here? Isn't a reference to a ZST 'static automatically?

amethyst_animation/src/bundle.rs, line 59 at r1 (raw file):

/// Bundle for only the sampler interpolation.
///
/// Will add `SamplerInterpolationSystem` with name `sampler_interpolation_system`.

sampler_processor isn't listed here

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

        builder: DispatcherBuilder<'a, 'b>,
    ) -> Result<DispatcherBuilder<'a, 'b>> {
        if !world

I think we have added an Entry API for this.

amethyst_animation/src/resources.rs, line 15 at r1 (raw file):

pub trait AnimationSampling {
    /// The channel type
    type Channel;

Maybe add the Send + Sync + 'static bounds here?

amethyst_animation/src/resources.rs, line 23 at r1 (raw file):

    /// Get the current sample for a channel
    fn get_current_sample(&self, channel: &Self::Channel) -> SamplerPrimitive<Self::Scalar>;

Should be current_sample

amethyst_animation/src/resources.rs, line 264 at r1 (raw file):

        command: AnimationCommand,
    ) -> Self {
        Self {

Please use AnimationControl for the construction

Comments from Reviewable

Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think we have added an Entry API for this.

Released?

Comments from Reviewable

Rhuagh · Feb 3, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think we have added an Entry API for this.

Released?

Comments from Reviewable

Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 16 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Why are you using a constant here? Isn't a reference to a ZST 'static automatically?

Done.

amethyst_animation/src/bundle.rs, line 59 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

sampler_processor isn't listed here

Done.

amethyst_animation/src/resources.rs, line 15 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Maybe add the Send + Sync + 'static bounds here?

Done.

amethyst_animation/src/resources.rs, line 23 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be current_sample

Done.

amethyst_animation/src/resources.rs, line 264 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please use AnimationControl for the construction

Done.

Comments from Reviewable

Rhuagh · Feb 3, 2018

Review status: all files reviewed at latest revision, 6 unresolved discussions.

amethyst_animation/src/bundle.rs, line 16 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Why are you using a constant here? Isn't a reference to a ZST 'static automatically?

Done.

amethyst_animation/src/bundle.rs, line 59 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

sampler_processor isn't listed here

Done.

amethyst_animation/src/resources.rs, line 15 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Maybe add the Send + Sync + 'static bounds here?

Done.

amethyst_animation/src/resources.rs, line 23 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Should be current_sample

Done.

amethyst_animation/src/resources.rs, line 264 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please use AnimationControl for the construction

Done.

Comments from Reviewable

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Released?

Yep, https://docs.rs/shred/0.5.2/shred/struct.Resources.html#method.entry

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

///
/// Will add `SamplerInterpolationSystem<T>` with the given name.
/// Will also add `SamplerProcessor<T::Scalar>` if it has not been added yet.

I think you misunderstood the usage of add(sys, "", dependencies). It does not check if it's already added, it will just add the same system twice.

Comments from Reviewable

torkleyy · Feb 4, 2018

Reviewed 8 of 8 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Released?

Yep, https://docs.rs/shred/0.5.2/shred/struct.Resources.html#method.entry

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

///
/// Will add `SamplerInterpolationSystem<T>` with the given name.
/// Will also add `SamplerProcessor<T::Scalar>` if it has not been added yet.

I think you misunderstood the usage of add(sys, "", dependencies). It does not check if it's already added, it will just add the same system twice.

Comments from Reviewable

Review status: 6 of 19 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yep, https://docs.rs/shred/0.5.2/shred/struct.Resources.html#method.entry

Done

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think you misunderstood the usage of add(sys, "", dependencies). It does not check if it's already added, it will just add the same system twice.

Ah, no, I just messed up the docs

Comments from Reviewable

Rhuagh · Feb 5, 2018

Review status: 6 of 19 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

amethyst_animation/src/bundle.rs, line 93 at r1 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Yep, https://docs.rs/shred/0.5.2/shred/struct.Resources.html#method.entry

Done

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

I think you misunderstood the usage of add(sys, "", dependencies). It does not check if it's already added, it will just add the same system twice.

Ah, no, I just messed up the docs

Comments from Reviewable

Review status: 6 of 19 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

amethyst_animation/src/resources.rs, line 62 at r3 (raw file):

/// Only required for animations which target more than a single node.
#[derive(Debug, Clone)]
pub struct AnimationHierarchy {

This should probably be generic on T aswell

Comments from Reviewable

Rhuagh · Feb 5, 2018

Review status: 6 of 19 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

amethyst_animation/src/resources.rs, line 62 at r3 (raw file):

/// Only required for animations which target more than a single node.
#[derive(Debug, Clone)]
pub struct AnimationHierarchy {

This should probably be generic on T aswell

Comments from Reviewable

Reviewed 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Ah, no, I just messed up the docs

Please correct it to "with the given name"

amethyst_animation/src/bundle.rs, line 97 at r3 (raw file):

            .res
            .entry()
            .or_insert_with(|| AssetStorage::<Sampler<T::Primitive>>::new());

Can coerce the new function to a closure.

Comments from Reviewable

torkleyy · Feb 5, 2018

Reviewed 13 of 13 files at r3.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Ah, no, I just messed up the docs

Please correct it to "with the given name"

amethyst_animation/src/bundle.rs, line 97 at r3 (raw file):

            .res
            .entry()
            .or_insert_with(|| AssetStorage::<Sampler<T::Primitive>>::new());

Can coerce the new function to a closure.

Comments from Reviewable

Xaeroxe · Feb 7, 2018

Xaeroxe approved these changes Feb 7, 2018

View changes

Impressive work! LGTM! I got a couple nits but I don't consider either of them blockers.

Xaeroxe · Feb 7, 2018

amethyst_animation/src/bundle.rs

+        Self {
+            animation_name,
+            sampling_name,
+            dep: &[],


I don't see anywhere this could be something other than &[] so why not just inline that and remove this member variable?

The with_dep function is hidden in the block below :)

Gotcha, thanks, feel free to ignore.

Xaeroxe · Feb 7, 2018

amethyst_animation/src/resources.rs

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


targets is the correct word here because this sentence is present tense.

Review status: 15 of 19 files reviewed at latest revision, 5 unresolved discussions.

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please correct it to "with the given name"

Done.

amethyst_animation/src/bundle.rs, line 97 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can coerce the new function to a closure.

Done.

amethyst_animation/src/resources.rs, line 62 at r3 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This should probably be generic on T aswell

Done.

amethyst_animation/src/resources.rs, line 72 at r3 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

targets is the correct word here because this sentence is present tense.

Done.

Comments from Reviewable

Rhuagh · Feb 7, 2018

Review status: 15 of 19 files reviewed at latest revision, 5 unresolved discussions.

amethyst_animation/src/bundle.rs, line 57 at r2 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Please correct it to "with the given name"

Done.

amethyst_animation/src/bundle.rs, line 97 at r3 (raw file):

Previously, torkleyy (Thomas Schaller) wrote…

Can coerce the new function to a closure.

Done.

amethyst_animation/src/resources.rs, line 62 at r3 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

This should probably be generic on T aswell

Done.

amethyst_animation/src/resources.rs, line 72 at r3 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

targets is the correct word here because this sentence is present tense.

Done.

Comments from Reviewable

All comments addressed now I believe.

Review status: 15 of 19 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

Rhuagh · Feb 7, 2018

All comments addressed now I believe.

Review status: 15 of 19 files reviewed at latest revision, 5 unresolved discussions.

Comments from Reviewable

omni-viral · Feb 8, 2018

omni-viral reviewed Feb 8, 2018

View changes

This is great.
Sorry for late review.
Only few nits.

omni-viral · Feb 8, 2018

amethyst_gltf/src/format/mod.rs

-        .collect::<Result<Vec<(usize, Sampler)>, GltfError>>()?
-        .into_iter()
-        .unzip();
+        .collect::<Result<Vec<(usize, LocalTransformChannel, Sampler<SamplerPrimitive<f32>>)>, GltfError>>()?;


Consider to use Result<Vec<_>, _> and let the rest to be inferred.

omni-viral · Feb 8, 2018

amethyst_gltf/src/format/mod.rs

             let output = AccessorIter::<[f32; 4]>::new(sampler.output(), buffers)
-                .map(|q| [q[3], q[0], q[1], q[2]])
+                .map(|q| SamplerPrimitive::Vec4([q[3], q[0], q[1], q[2]]))


Could it be handled by From trait?

No, because it needs to be swizzled

Or hmm, it might

omni-viral · Feb 8, 2018

amethyst_gltf/src/systems.rs

                                 .samplers
                                 .iter()
                                 .cloned()
-                                .map(|sampler| {
-                                    loader.load_from_data(sampler, (), &*sampler_storage)
+                                .map(|(node_index, channel, ref sampler)| {


Why ref sampled if it is cloned?
sampler now cloned two times.

Review status: 15 of 19 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

amethyst_gltf/src/systems.rs, line 234 at r4 (raw file):

Previously, omni-viral (Zakarum) wrote…

Why ref sampled if it is cloned?
sampler now cloned two times.

Done.

amethyst_gltf/src/format/mod.rs, line 187 at r4 (raw file):

Previously, omni-viral (Zakarum) wrote…

Consider to use Result<Vec<_>, _> and let the rest to be inferred.

Done.

amethyst_gltf/src/format/mod.rs, line 229 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Or hmm, it might

Done.

Comments from Reviewable

Rhuagh · Feb 8, 2018

Review status: 15 of 19 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

amethyst_gltf/src/systems.rs, line 234 at r4 (raw file):

Previously, omni-viral (Zakarum) wrote…

Why ref sampled if it is cloned?
sampler now cloned two times.

Done.

amethyst_gltf/src/format/mod.rs, line 187 at r4 (raw file):

Previously, omni-viral (Zakarum) wrote…

Consider to use Result<Vec<_>, _> and let the rest to be inferred.

Done.

amethyst_gltf/src/format/mod.rs, line 229 at r4 (raw file):

Previously, Rhuagh (Simon Rönnberg) wrote…

Or hmm, it might

Done.

Comments from Reviewable

r? @torkleyy @omni-viral @Xaeroxe

Rhuagh · Feb 9, 2018

r? @torkleyy @omni-viral @Xaeroxe

omni-viral · Feb 9, 2018

omni-viral reviewed Feb 9, 2018

View changes

Rhuagh · Feb 9, 2018

Rhuagh dismissed omni-viral’s stale review Feb 9, 2018

is implicit approval

amethyst_animation/src/resources.rs, line 67 at r5 (raw file):

}

macro_rules! hashmap {

Cool macro! We should consider moving this to core.

Comments from Reviewable

Xaeroxe · Feb 9, 2018

amethyst_animation/src/resources.rs, line 67 at r5 (raw file):

}

macro_rules! hashmap {

Cool macro! We should consider moving this to core.

Comments from Reviewable

Xaeroxe · Feb 9, 2018

Xaeroxe approved these changes Feb 9, 2018

View changes

!

bors r+

Reviewed 3 of 4 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

amethyst_animation/src/resources.rs, line 67 at r5 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Cool macro! We should consider moving this to core.

There is some crate that exposes such a macro..

Comments from Reviewable

torkleyy · Feb 9, 2018

bors r+

Reviewed 3 of 4 files at r4, 2 of 2 files at r5.
Review status: all files reviewed at latest revision, all discussions resolved.

amethyst_animation/src/resources.rs, line 67 at r5 (raw file):

Previously, Xaeroxe (Jacob Kiesel) wrote…

Cool macro! We should consider moving this to core.

There is some crate that exposes such a macro..

Comments from Reviewable

torkleyy · Feb 9, 2018

torkleyy approved these changes Feb 9, 2018

View changes

Build failed

continuous-integration/travis-ci/push

bors · Feb 9, 2018

Build failed

continuous-integration/travis-ci/push

bors r+

torkleyy · Feb 9, 2018

bors r+

Build succeeded

bors · Feb 9, 2018

Build succeeded

Rhuagh added type: improvement status: working project: animation labels Feb 1, 2018

Rhuagh changed the title from [WIP] feat: Generic animation system to feat: Generic animation system Feb 2, 2018

Rhuagh added status: ready and removed status: working labels Feb 2, 2018

omni-viral self-requested a review Feb 6, 2018

feat: Generic animation system

Loading status checks…

5cec9de

Rhuagh dismissed omni-viral’s stale review Feb 9, 2018

Xaeroxe referenced this pull request Feb 9, 2018
Merged
Rename transforms #563

Rhuagh deleted the Rhuagh:feature/animation-generic branch Feb 9, 2018

amethyst/amethyst

Join GitHub today

feat: Generic animation system #558

Conversation

Rhuagh commented Feb 1, 2018 • edited Edited 5 times Rhuagh edited Feb 2, 2018 (most recent) Rhuagh edited Feb 2, 2018 Rhuagh edited Feb 2, 2018 Rhuagh edited Feb 1, 2018 torkleyy edited Feb 1, 2018

Rhuagh added type: improvement status: working project: animation labels Feb 1, 2018

Rhuagh changed the title from [WIP] feat: Generic animation system to feat: Generic animation system Feb 2, 2018

Rhuagh added status: ready and removed status: working labels Feb 2, 2018

This comment has been minimized.

Rhuagh commented Feb 2, 2018

This comment has been minimized.

torkleyy commented Feb 3, 2018

This comment has been minimized.

Rhuagh commented Feb 3, 2018

This comment has been minimized.

Rhuagh commented Feb 3, 2018

This comment has been minimized.

torkleyy commented Feb 4, 2018

This comment has been minimized.

Rhuagh commented Feb 5, 2018

This comment has been minimized.

Rhuagh commented Feb 5, 2018

This comment has been minimized.

torkleyy commented Feb 5, 2018

omni-viral self-requested a review Feb 6, 2018

Xaeroxe approved these changes Feb 7, 2018 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.

Rhuagh commented Feb 7, 2018

This comment has been minimized.

Rhuagh commented Feb 7, 2018

omni-viral reviewed Feb 8, 2018 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.

Rhuagh commented Feb 8, 2018

This comment has been minimized.

Rhuagh commented Feb 9, 2018

omni-viral reviewed Feb 9, 2018 View changes

Rhuagh dismissed omni-viral’s stale review Feb 9, 2018

This comment has been minimized.

Xaeroxe commented Feb 9, 2018

Xaeroxe approved these changes Feb 9, 2018 View changes

This comment has been minimized.

torkleyy commented Feb 9, 2018

bors bot added a commit that referenced this pull request Feb 9, 2018

torkleyy approved these changes Feb 9, 2018 View changes

This comment has been minimized.

bors bot Feb 9, 2018

Build failed

bors bot commented Feb 9, 2018

Build failed

This comment has been minimized.

torkleyy commented Feb 9, 2018

bors bot added a commit that referenced this pull request Feb 9, 2018

This comment has been minimized.

bors bot Feb 9, 2018

Build succeeded

bors bot commented Feb 9, 2018

Build succeeded

Hide details View details bors bot merged commit 5cec9de into amethyst:develop Feb 9, 2018 4 checks passed

4 checks passed

Xaeroxe referenced this pull request Feb 9, 2018

Rhuagh deleted the Rhuagh:feature/animation-generic branch Feb 9, 2018

Rhuagh commented Feb 1, 2018 •

edited

Edited 5 times

Rhuagh edited Feb 2, 2018 (most recent)

Rhuagh edited Feb 2, 2018

Rhuagh edited Feb 2, 2018

Rhuagh edited Feb 1, 2018

torkleyy edited Feb 1, 2018

Xaeroxe approved these changes Feb 7, 2018

View changes

omni-viral reviewed Feb 8, 2018

View changes

omni-viral reviewed Feb 9, 2018

View changes

Xaeroxe approved these changes Feb 9, 2018

View changes

torkleyy approved these changes Feb 9, 2018

View changes

bors bot merged commit `5cec9de` into amethyst:develop Feb 9, 2018
4 checks passed