SpriteRenderComponent #211
Conversation
There was a problem hiding this comment.
Nice!
Besides the comments I left on the code, also note the failing tests (you can see them in the Mac CI build link, here: https://travis-ci.org/tzachshabtay/MonoAGS/jobs/331353522)
| /// <summary> | ||
| /// An animation container. This gives access to the animation, and allows to start a new animation | ||
| /// which will replace the old animation. | ||
| /// </summary> | ||
| [RequiredComponent(typeof(ISpriteRenderComponent))] |
There was a problem hiding this comment.
Not a big deal, as we're not actually using the RequiredComponent yet, so just and FYI, placing the attribute on the interfaces was a mistake on my part. The component dependencies should be on the implementing class, not on the interface (for example, an alternative implementation of the animation component might not need the sprite component). Once I get to actually using it (in #185 ), I plan on moving all the attributes from the interfaces to the implementations.
| /// Allows to subscribe to the animation frame change. | ||
| /// </summary> | ||
| // TODO: Should this be a blocking event? | ||
| IEvent OnNextFrame { get; } |
There was a problem hiding this comment.
You don't need to add this event: IAnimationState implements INotifyPropertyChanged, so you can use the PropertyChanged event instead, and check for the CurrentFrame property change:
animationState.PropertyChanged += onAnimationStatePropertyChanged;
private void onAnimationStatePropertyChanged(object sender, PropertyChangedEventArgs args)
{
if (args.PropertyName != nameof(IAnimationState.CurrentFrame)) return;
//... change sprite here ...
}Note that you will not find the code that actually fires that event, that's because I'm using https://github.com/Fody/PropertyChanged for injecting the code on compile.
| if (currentAnimation != null) | ||
| { | ||
| currentAnimation.State.OnNextFrame.Subscribe(OnAnimationFrame); | ||
| currentAnimation.State.OnAnimationCompleted.TrySetResult(new AnimationCompletedEventArgs(false)); |
There was a problem hiding this comment.
This is a mistake: currentAnimation holds the currently playing animation, the one we're about to replace with the new animation. So you should not subscribe to currentAnimation here, you should unsubscribe currentAnimation and subscribe to animation -> this probably explains the glitches you've seen.
|
|
||
| private void OnAnimationFrame() | ||
| { | ||
| // TODO: perhaps instead checking this every time, subscribe and unsubscribe |
There was a problem hiding this comment.
I did not understand the todo comment: the on frame event (or rather, the property changed event as I mentioned above) is in animation state, part of the animation which is not a component: it's a parameter to the StartAnimation method, so there's no component add/remove for subscribing?
There was a problem hiding this comment.
What I meant here is that there possibly is a way to ensure this callback is only called if _spriteRender is not null (therefore proceed without conditional check):
- do not subscribe to the event unless sprite component is present;
- subscribe to the event when sprite component is added (if animation is already running);
- unsubscribe from the event if sprite component gets removed.
But this depends ofcourse on whether component may be added/removed in the period of time between event firing and callback running (or not).
|
First contribution, I'm very excited :) Regarding your comments here:
|
|
Cool, only thing missing is fixing the tests. |
|
Following our discussion, latest commit introduces ISpriteProvider interface for SpriteRenderComponent to get active sprite from. Component itself does not have sprite setter anymore. Something I wanted to ask, do you prefer that pull request commits to be squashed eventually (hiding code rewrite iterations), or want to keep them all? |
|
I usually prefer to keep the commits, unless you have any objections. Ok, looks good and the tests pass. Good to merge? |
Well, I used to recreate WIP branches in case when the final code changed too much along the way compared to initial idea, because that could cause problems understanding what has changed and why, especially when searching for an origin of a bug. For that reason I'd rather clean it up first, it should take a little time.
Test failure still a mistery to me, last time I checked there was something related to savegame serialization, which I have zero knowledge about. Somehow it does not happen now? |
Sure, but I mean, do you have any objections of keeping the commits for this PR? Or do you want me to squash? Or do you want to recreate?
It looks fine now, and you did change stuff for serialization (you removed the sprite from the contract), so it was possibly related to that. I wouldn't worry too much about it, the serialization is not in a workable state now anyway and would be completely revamped soon(ish). |
Yes, I would like to recreate it, if you don't mind, that should take 15 mins at most.
Actually, I noticed I forgot to remove a Sprite property from ContractSpriteRenderComponent, it just not used in serialization, but still declared. |
|
Ok. |
|
I have to ask, what fields need to be in this ProtoContract? There are fields in the component that are set freely, like Border and DebugDrawPivot, there is readonly field CurrentSprite, and also SpriteProvider. |
|
The proto contract is everything that's saved as part of a saved game. CurrentSprite is taken from the provider, so no need to save it. The other 3 need to be saved. |
This adds ISpriteRenderComponent interface and its AGSSpriteRenderComponent implementation. SpriteRenderer is supposed to become a rendering "contractor" for 2D sprite-based components, such as ImageComponent or AnimationComponent. SpriteRenderComponent stores a reference to ISpriteProvider, from which it receives "current sprite" to draw. AGSAnimationComponent implements ISpriteProvider. Moved Border and DebugDrawPivot properties from Animation to SpriteRender component. Made GLImageRenderer refer to SpriteRenderComponent for drawing (instead of AnimationComponent). It is assumed, that parts of GLImageRenderer rendering function will be merged into SpriteRenderComponent on later stages.
ivan-mogilko
force-pushed
the
refact--spriterender
branch
from
17c0656 to
95b565a
Compare
|
Well, I think I'm done. |
|
Yeah, the linux build is not currently working, you can ignore that. The tests pass, merging. |
For #206.
This adds ISpriteRenderComponent interface and its AGSSpriteRenderComponent implementation.
SpriteRenderer is supposed to become a focal point of image-based objects (and eventually implement rendering itself), having its properties updated by others, such as AnimationComponent.
Moved Border and DebugDrawPivot properties from Animation to SpriteRender component.
Had to add OnNextFrame event to IAnimation to let AnimationComponent know when to update renderer.
NOTE 1:
When I was about to create this PR, the doubts came to me, and I thought that I might be doing something wrong.
Initially, I proposed this component as a medium between renderer and 2D image-based components, but since we decided there will be IRenderer interface, that components could may implement themselves (e.g. mentioned in #207), does that make SpriteComponent redundant? Because, to think of it, AnimationComponent and ImageComponent (especially latter one) can implement IRenderer too.
I am mostly wondering if ImageComponent and SpriteRenderComponent are essentially duplicating each other, or not.
NOTE 2:
Current implementation is a first try. There were few things I had doubts about in technical sense (added TODO comments for these).
First of all, template generation failed for me for some reason, introducing unexpected changes to generated classes. I will try to investigate again, but for now I just implemented the necessary changes by hand.
Secondly, since AnimationComponent is not rendered directly, but has to update SpriteRenderComp., I had to somehow let it know that animation frame changes, so added new event to IAnimation. The first tests in DemoGame show that rendering works, but there were couple of possible glitches, and I am not sure whether my code covered all possible cases when animation may be updated.