prime31 / Nez

Nez is a free 2D focused framework that works with MonoGame and FNA
MIT License
1.77k stars 360 forks source link

Sprite draws incorrectly when using flipX with animation containing different frame origins #120

Closed Eendhoorn closed 7 years ago

Eendhoorn commented 7 years ago

The sprite seems to draw incorrectly when using sprite.flipX and using an animation that has different origins between the frames. I took a look into the code, but I do not dare to touch the batcher haha.

pecker_offset

animation_frames

animation_frames_tech

prime31 commented 7 years ago

I can't seem to reproduce this. I've got a few animations that have origins changing though they are per animation as opposed to within a single animation.

One thing that will help determine if the issue is in the Batcher is to just change the Sprite.render method to use a SpriteBatch instead. See if that has the same rendering result. Also, if you have a small tester project that you can send over that would be helpful as well.

var sb = new SpriteBatch( Core.graphicsDevice );
sb.Start( transformMatrix:camera.transformMatrix );
sb.Draw( _subtexture, entity.transform.position + localOffset, _subtexture.sourceRect, color, entity.transform.rotation, origin, entity.transform.scale, spriteEffects, _layerDepth );
sb.End();
selimanac commented 7 years ago

@prime31 Same here. sample is attached.

NezPlatformer.zip

Eendhoorn commented 7 years ago

@prime31 Replacing the Batcher with the SpriteBatch yields the same results (apart from being a bit blurry). I've attached a small sample below; NezSpriteFlipSample.zip

Thanks!

@selimanac I can't seem to open your sample to take a look myself, visual studio is complaining about version incompatabillity :(

prime31 commented 7 years ago

So, SpriteBatch definitely matches Batcher here. That's good. Rendering is working correctly. Digging deeper it appears that the issue here is that all the origins are identical as you can see from the debug view of the frames:

screen shot 2016-10-04 at 5 28 30 pm

Looking into the TexturePacker JSON file it would appear the origins are being calculated correctly. Doing them out by hand results in a match as you can see here:

screen shot 2016-10-04 at 5 33 38 pm

Eendhoorn commented 7 years ago

I took a look at the batcher to see how it uses the origin, but it's all just way over my head to understand in a quick glance haha. Even though the origins are almost similar,it's rendering correctly when the sprite is not flipped.

I added a debug rect to the frame animation and it made the problem a bit more apparant pecker_offset_debug

I'm not sure if I'm right, but the sprite seems to be drawn from the top-right, so when you flip it the distance from the center of the sprite becomes incorrect.

I was able to fix this by calculating the origin from the right side of the sprite instead of the left side when it's flipped.

public Sprite setSubtexture( Subtexture subtexture )
{
    _subtexture = subtexture;

    if ( _subtexture != null )
    {
        _origin = subtexture.origin;
        if ( flipX ) _origin.X = subtexture.sourceRect.Width - subtexture.origin.X;
    }
    return this;
}

pecker_offset_debug_correct

I'm not sure if this can be consired a proper fix though

prime31 commented 7 years ago

This is a tricky one. I am not 100% certain what the best course of action is, if any. The Batcher should match SpriteBatch as much as possible for now (in the future it may go it's own way). SpriteBatch seems to just flip as-is whether there is a non-centered origin or not so Batcher should match that.

On a higher level this can be addressed in the Sprite class whenever flipX/flipY are set. If SpriteEffects.FlipHorizontally/Vertically is set and origin is not centered it can be adjusted. If flipX/flipY is set to false the origin can be reset to Subtexture.origin.

I'll have to give it some thought to decide if this should be automatic or optional. I am thinking a bool to control whether it gets done automatically might be a good idea.

Eendhoorn commented 7 years ago

Yeah I can agree that it's best to keep the batcher and the spritebatch as similar as possible, I can imagine some people might want to try porting existing projects to Nez.

Just changing it on the flipX wouldn't work right? It would break when the sprite enters a new animation frame.

I'll just keep this code for now until you've decided what the best way to handle this is. I just needed a quick fix right now since I need to upload a build for a gamejam soon.

prime31 commented 7 years ago

You are correct. It would need to be changed at flipX and in setSubtexture.

bfloch commented 7 years ago

Any updates on this? Also how did the offset ever work properly, even when not flipped? The TexturePackerImporter does not seem to propagate any information of the offset in regard to the original sprite size.

prime31 commented 7 years ago

There is no right answer for this so the best thing to do is just subclass and add/subtract the offset yourself when you switch subtextures. Or you could just set the localOffset when you change the subtexture. Or you could center your artwork and not do anything at all with offsets. Tons of options here.

Eendhoorn commented 7 years ago

@bfloch

I'm still running this piece of code that fixed it.

public Sprite setSubtexture( Subtexture subtexture )
{
    _subtexture = subtexture;

    if ( _subtexture != null )
    {
        _origin = subtexture.origin;
        if ( flipX ) _origin.X = subtexture.sourceRect.Width - subtexture.origin.X;
    }
    return this;
}

Also, be sure to set Trim mode to "Crop, keep position" in texturepacker!

Never had a any problems, but I didn't want to a pull request on something so essential.

bfloch commented 7 years ago

Thanks Eendhoorn. I haven't done the keep position, mainly because I felt I wanted to keep the origins meaningful.

I still believe that NEZ, as is, does not support TexturePacker completely (that is including a proper origin, regular and flipped).

The whole point of texture packer is to reduce texture memory and overdraw. The assumption is that this is a higher cost than doing the offset calculations. "Center your artwork" seems like a counter intuitive suggestion with that in mind.

Anyhow I would still suggest to add the option to pass the source size to the subtexture (can be same as regular width in non trimmed texture atlases), then the origin can be calculated correctly in-engine.

Cheers.