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 stepping and exact input value control to animation system #569

Merged
merged 1 commit into from Feb 13, 2018

Conversation

Projects
None yet
3 participants
@Rhuagh
Member

Rhuagh commented Feb 11, 2018

Needs rebase when #567 is merged.


This change is Reviewable

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 12, 2018

Member

Rebased, ready now.

Member

Rhuagh commented Feb 12, 2018

Rebased, ready now.

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 12, 2018

Member

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


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

/// Used when doing animation stepping (i.e only move forward/backward to discrete input values)
#[derive(Clone, Debug)]
pub enum StepDirection {

Can you make the repr to i8 and set Forward to +1 and Backward to -1? That might make the code above shorter.


Comments from Reviewable

Member

torkleyy commented Feb 12, 2018

Reviewed 5 of 5 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


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

/// Used when doing animation stepping (i.e only move forward/backward to discrete input values)
#[derive(Clone, Debug)]
pub enum StepDirection {

Can you make the repr to i8 and set Forward to +1 and Backward to -1? That might make the code above shorter.


Comments from Reviewable

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 12, 2018

Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, torkleyy (Thomas Schaller) wrote…

Can you make the repr to i8 and set Forward to +1 and Backward to -1? That might make the code above shorter.

Not sure, still need to do edge case handling individually so the index don't overflow/underflow.


Comments from Reviewable

Member

Rhuagh commented Feb 12, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, torkleyy (Thomas Schaller) wrote…

Can you make the repr to i8 and set Forward to +1 and Backward to -1? That might make the code above shorter.

Not sure, still need to do edge case handling individually so the index don't overflow/underflow.


Comments from Reviewable

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
@Rhuagh

Rhuagh Feb 12, 2018

Member

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, Rhuagh (Simon Rönnberg) wrote…

Not sure, still need to do edge case handling individually so the index don't overflow/underflow.

It would remove one line, that's about it.


Comments from Reviewable

Member

Rhuagh commented Feb 12, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, Rhuagh (Simon Rönnberg) wrote…

Not sure, still need to do edge case handling individually so the index don't overflow/underflow.

It would remove one line, that's about it.


Comments from Reviewable

@Rhuagh

This comment has been minimized.

Show comment
Hide comment
Member

Rhuagh commented Feb 12, 2018

@torkleyy

This comment has been minimized.

Show comment
Hide comment
@torkleyy

torkleyy Feb 13, 2018

Member

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

Member

torkleyy commented Feb 13, 2018

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@Xaeroxe

LGTM!

bors r+

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

Merge #569
569:  feat: Add stepping and exact input value control to animation system r=Xaeroxe a=Rhuagh

Needs rebase when #567 is merged.

<!-- 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/569)
<!-- Reviewable:end -->
@bors

This comment has been minimized.

Show comment
Hide comment

@bors bors bot merged commit 6fc5ca1 into amethyst:develop Feb 13, 2018

4 checks passed

bors Build succeeded
code-review/reviewable 5 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-stepping branch Feb 13, 2018

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