SpriteRenderComponent

[ghost]

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.

[tzachshabtay]

First contribution, I'm very excited :)

Regarding your comments here:

1. See my review above regarding the event and the glitches.

2. I don't think the sprite component is redundant, like you observed in our earlier discussion, the animation component should be just about handling the state, and not actually handling the visuals so we wouldn't want it to implement IRenderer itself.

3. I've neglected the template generation lately as I've been working on Mac and there's a bug in the template generation in VS for Mac, so I've also been updating manually. Very possible that there are bugs there, if you want to investigate them, that's cool, otherwise I'll investigate when I finish my current work.

[tzachshabtay]

Cool, only thing missing is fixing the tests.

[ghost]

Following our discussion, latest commit introduces ISpriteProvider interface for SpriteRenderComponent to get active sprite from. Component itself does not have sprite setter anymore. Currently only AnimationComponent implements ISpriteProvider (because ImageComponent works through animations too).

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?

[tzachshabtay]

I usually prefer to keep the commits, unless you have any objections. Ok, looks good and the tests pass. Good to merge?

[ghost]

I usually prefer to keep the commits, unless you have any objections.

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.

Ok, looks good and the tests pass.

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?

[tzachshabtay]

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.

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? I'm fine with whatever you prefer.

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?

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).

[ghost]

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?

Yes, I would like to recreate it, if you don't mind, that should take 15 mins at most.

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).

Actually, I noticed I forgot to remove a Sprite property from ContractSpriteRenderComponent, it just not used in serialization, but still declared.

[tzachshabtay]

Ok.

[ghost]

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.

[tzachshabtay]

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.

[ghost]

Well, I think I'm done. Some builds failed, but logs say something about missing nuget package, so idk.

[tzachshabtay]

Yeah, the linux build is not currently working, you can ignore that. The tests pass, merging.