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

Refactor Anim/SpriteRenderer #439

Closed
ilexp opened this issue Nov 19, 2016 · 10 comments

Comments

1 participant
@ilexp
Copy link
Member

commented Nov 19, 2016

Summary

Having AnimSpriteRenderer derive from SpriteRenderer is bad design from a component-based perspective and annoying when introducing a new subclass, as there always have to be two of them - one animated and one not. To fix this, get rid of AnimSpriteRenderer entirely and let SpriteRenderer expose a sprite index property.

Analysis

  • All animation functionality can be moved to a new SpriteAnimator component that requires a SpriteRenderer and sets the displayed sprite index accordingly. This is a much cleaner design the separates these features horizontally, rather than stacking them vertically.
  • Smooth animation can be removed entirely, see issue #349.

@ilexp ilexp added this to the v3.0 milestone Nov 19, 2016

@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2016

Note: Should the basic SpriteRenderer already expose properties for two simultaneously displayed, blended frames and just ignore them, while subclasses may use that functionality?

Note: Alternatively, should there be an interface between SpriteRenderer and SpriteAnimator that exposes these properties, which is used instead of a direct SpriteRenderer implementation?

If none of the above was implemented, this new design would be limited to hard, instantaneous switches between a finite number of animation states, which is likely insufficient in some advanced cases such as smooth animation. It would also be possible to introduce a new SpriteIndexBlend struct with two indices and a float blend.

Note: Should the API be consciously limited to hard, instantaneous switches as described above?

@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 18, 2016

Progress

  • Started working on this in the wip-spriteanimator branch.
  • Extracted SpriteAnimator class.
  • Removed AnimSpriteRenderer class.
  • Introduced a new ICmpSpriteRenderer component interface that allows to set a SpriteIndexBlend.
  • The SpriteRenderer component now exposes a SpriteIndex property of the above index blend type.
  • Implemented a SpriteIndexBlend property editor that uses a horizontal layout in a single line, rather than the default expandable sub editor.
  • The SmoothAnimation sample implements a special SpriteRenderer, but doesn't touch animation at all anymore.
  • The DynamicLighting samples have been adjusted similarly. No animation re-implementation.
  • Right now, all affected samples are broken content-wise and have to be updated as well.

Immediate ToDo

  • Validate SpriteIndex state when adjusting SpriteAnimator properties. Previously it was done on every draw and update. Now that draw is no longer there, adjusting properties in the editor has no visible effect. Fix this!
    • Option A: Update on property set (and make the update method private, as it would be no longer needed)
    • Option B: Update in editor update.
  • See if there is a way to lock or update the SpriteIndex property editor somehow if there's an animator present. As far as updating goes, a generic way to trigger "affects property X" updates could be introduced to the property grid via EditorHint attribute?
  • Add a note to SpriteRenderer docs that SpriteIndex does not support displaying blended indices and thus, blending values will be ignored.
  • Either do not remove blending data from SpriteRenderer sprite index or find a way to not expose the full SpriteIndexBlend struct due to lack of blend support. Having it there while not being able to edit it feels a bit buggy / unpolished.
    • Another option would be to have the blend and next index editors hidden by default, unless blend is non-zero, or a checkbox in the editor is set.
    • Also, is there a way to make these VectorPropertyEditors more appealing? Color-coding individual editors, adding optional labels? Would be something for the master branch to be merged back to the dev-3.0.
  • Use a different icon for SpriteAnimator.
  • Update DynamicLighting sample content.
  • Update SmoothAnimation sample content.
  • Update DualStickSpaceShooter sample content.
  • Update FlapOrDie sample content.
  • Update ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Update ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.
  • Evaluate the changes and make adjustments where necessary.
  • Merge back to develop-3.0.

@ilexp ilexp self-assigned this Dec 18, 2016

@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2016

Progress

  • Now updating animation states whenever adjusting a directly related property on SpriteAnimator.
  • Discarded the idea of syncing SpriteRenderer.SpriteIndex in the property grid, as it's sort of a nice2have feature that requires a bigger systemic change to be introduced properly. Postponing this.
  • Updated SpriteRenderer.SpriteIndex XML docs to indicate that the base class does not evaluate blending states. It also no longer removes blending info, so subclasses can use it.

Immediate ToDo

  • Update SpriteIndexBlend property editor to hide blend and next editors by default, unless blend is non-zero, or a checkbox in the editor is set.
    • Also, is there a way to make these VectorPropertyEditors more appealing? Color-coding individual editors, adding optional labels? Would be something for the master branch to be merged back to the dev-3.0.
  • Use a different icon for SpriteAnimator.
  • Update DynamicLighting sample content.
  • Update SmoothAnimation sample content.
  • Update DualStickSpaceShooter sample content.
  • Update FlapOrDie sample content.
  • Update ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Update ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.
  • Evaluate the changes and make adjustments where necessary.
  • Merge back to develop-3.0.
@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2016

Progress

  • Added an editor icon for SpriteAnimator.

Immediate ToDo

  • Update SpriteIndexBlend property editor to hide blend and next editors by default, unless blend is non-zero, or a checkbox in the editor is set.
    • Also, is there a way to make these VectorPropertyEditors more appealing? Color-coding individual editors, adding optional labels? Would be something for the master branch to be merged back to the dev-3.0.
  • Rename SpriteAnimator properties. Now that it's a distinct component there's no reason for them all to begin with Anim.
  • Check if it makes sense to extend the default SpriteAnimator so it could replace the Tilemaps samples' ActorAnimator
  • Update ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Update ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.
  • Update DynamicLighting sample content.
  • Update SmoothAnimation sample content.
  • Update DualStickSpaceShooter sample content.
  • Update FlapOrDie sample content.
  • Evaluate the changes and make adjustments where necessary.
  • Merge back to develop-3.0.
@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

Progress

  • Updated content for FlapOrDie, SmoothAnimation, BasicMenu and DynamicLighting samples.
  • Lots of bugfixes.

Immediate ToDo

  • Update DualStickSpaceShooter sample content.
  • Extend RequiredComponent to be able to handle interfaces and abstract classes, combined with a "default type" that will be used for auto-created dependency components. That way, the SpriteAnimator can, by default, auto-generate a SpriteRenderer. This addition can be done in master and then merged back up.
  • Introduce an implicit conversion from int to SpriteIndexBlend, update all usages and add a comment regarding this to the SpriteIndex property.
  • Rename SpriteAnimator properties. Now that it's a distinct component there's no reason for them all to begin with Anim.
  • Update ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Update ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.
  • Check if it makes sense to extend the default SpriteAnimator so it could replace the Tilemaps samples' ActorAnimator
  • Update SpriteIndexBlend property editor to hide blend and next editors by default, unless blend is non-zero, or a checkbox in the editor is set.
    • Also, is there a way to make these VectorPropertyEditors more appealing? Color-coding individual editors, adding optional labels? Would be something for the master branch to be merged back to the dev-3.0.
  • Merge back to develop-3.0.
@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 21, 2016

Note: Consider getting rid of SpriteIndexBlend completely, as flipbook animation is just the way more common case and the smooth animation sample can do a custom subclass implementation of both renderer and animator. This would simplify a lot of things, including user experience / learning.

When doing this, update FlapOrDie and DualStickSpaceShooter accordingly.

@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 22, 2016

Progress

  • Updated content for DualStickSpaceShooter sample.
  • More bugfixes.

Immediate ToDo

  • Get rid of SpriteIndexBlend and make the SpriteIndex property a simple int.
    • Update FlapOrDie and DualStickSpaceShooter sample data.
    • Update SmoothAnimation sample to work with a special version of both SpriteRenderer and SpriteAnimator
  • Extend RequiredComponent to be able to handle interfaces and abstract classes, combined with a "default type" that will be used for auto-created dependency components. That way, the SpriteAnimator can, by default, auto-generate a SpriteRenderer. This addition can be done in master and then merged back up.
  • Rename SpriteAnimator properties. Now that it's a distinct component there's no reason for them all to begin with Anim.
  • Update ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Update ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.
  • Check if it makes sense to extend the default SpriteAnimator so it could replace the Tilemaps samples' ActorAnimator
  • Merge back to develop-3.0.
@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 24, 2016

Moved RequiredComponent extension to issue #464.

@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 25, 2016

Progress

  • ICmpSpriteRenderer.SpriteIndex is now a regular int.
  • Added ICmpSpriteRenderer.ApplySpriteAnimation method that provides additional info.
  • Updated FlapOrDie and DualStickSpaceShooter sample data.
  • Renamed some SpriteAnimator properties and fields, so they don't all contain the anim prefix for no good reason anymore.
  • Updated ActorRenderer in the tilemaps sample to implement ICmpSpriteRenderer.
  • Updated ActorAnimator to target an ICmpSpriteRenderer rather than an ActorRenderer.

Immediate ToDo

  • Resolve issue #464.
  • Update SpriteAnimator and ActorAnimator with a RequiredComponent attribute.
  • Merge back to develop-3.0.
@ilexp

This comment has been minimized.

Copy link
Member Author

commented Dec 27, 2016

Done.

@ilexp ilexp closed this Dec 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.