/amethyst

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

feat: Add low level support for animation blending #570

Merged
merged 2 commits into from Feb 15, 2018

Conversation

Reviewers

@Xaeroxe Xaeroxe

@torkleyy torkleyy

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@Rhuagh @torkleyy @Xaeroxe
@Rhuagh
Member

Rhuagh commented Feb 13, 2018 •
edited by torkleyy
Edited 1 time
  • @torkleyy torkleyy edited Feb 13, 2018 (most recent)

Still requires the user to do all blend weight handling.

This change is Reviewable
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

bors try+
Member

Rhuagh commented Feb 14, 2018

bors try+

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

@bors[bot]
Try #570:+
Loading status checks…
d8bf18c
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Feb 14, 2018

Contributor

try

Build failed
Contributor

bors bot commented Feb 14, 2018

try

Build failed
@Xaeroxe

Xaeroxe requested changes Feb 14, 2018
View changes

Impressive work! The newly added I generic doesn't portray a clear purpose though until you see it in use. Can we document the generic types a bit better?

amethyst_animation/src/resources.rs
@@ -13,13 +14,16 @@ pub trait AnimationSampling: Send + Sync + 'static {
/// The interpolation primitive
type Primitive: InterpolationPrimitive + Clone + Copy + Send + Sync + 'static;
/// The channel type
- type Channel: Clone + Hash + Eq + Send + Sync + 'static;
+ type Channel: Debug + Clone + Hash + Eq + Send + Sync + 'static;

This comment has been minimized.

Show comment
Hide comment
@Xaeroxe

Xaeroxe Feb 14, 2018

Member

Debug bound may not be necessary.
@Xaeroxe

Xaeroxe Feb 14, 2018

Member

Debug bound may not be necessary.

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

I could put it here or on every place where the channel is used, it's required for the use in AnimationCommand
@Rhuagh

Rhuagh Feb 14, 2018

Member

I could put it here or on every place where the channel is used, it's required for the use in AnimationCommand
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

I'll do a documentation pass
Member

Rhuagh commented Feb 14, 2018

I'll do a documentation pass
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

I'm also gonna add a function on the sampling trait for blending config (disabling it etc)
Member

Rhuagh commented Feb 14, 2018

I'm also gonna add a function on the sampling trait for blending config (disabling it etc)
@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

@Xaeroxe I have done a doc pass, and also added a way to disable animation blending on a per target component+channel basis
Member

Rhuagh commented Feb 14, 2018

@Xaeroxe I have done a doc pass, and also added a way to disable animation blending on a per target component+channel basis
@Rhuagh
feat: Sampler blending
Loading status checks…
db394c9
@Xaeroxe

Xaeroxe approved these changes Feb 14, 2018
View changes

Much better! Thanks!

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 14, 2018

Member

r? @torkleyy @omni-viral
Member

Rhuagh commented Feb 14, 2018

r? @torkleyy @omni-viral
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 15, 2018

Member

:lgtm:

Also great to see some more docs!

Reviewed 3 of 10 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable
Member

torkleyy commented Feb 15, 2018

:lgtm:

Also great to see some more docs!

Reviewed 3 of 10 files at r1, 7 of 7 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable
@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 15, 2018

Member

bors r+
Member

torkleyy commented Feb 15, 2018

bors r+

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

@bors[bot]
Merge #570 
570: feat: Add low level support for animation blending r=torkleyy a=Rhuagh

Still requires the user to do all blend weight handling.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/amethyst/amethyst/570)
<!-- Reviewable:end -->
Loading status checks…
f7ae74c
@bors

This comment has been minimized.

Show comment
Hide comment
@bors

bors bot Feb 15, 2018

Contributor

Build succeeded
Contributor

bors bot commented Feb 15, 2018

Build succeeded

@bors bors bot merged commit db394c9 into amethyst:develop Feb 15, 2018
4 checks passed

4 checks passed

bors Build succeeded
code-review/reviewable 10 files reviewed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Rhuagh Rhuagh deleted the Rhuagh:feature/animation-multi-sampler branch Apr 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment