[#715] texture atlas origins (anchorPoints)

[Giwayume]

I created two new properties for a texture's atlas:

{boolean} hasTextureAnchorPoint - Whether or not the current frame defines its own anchor point.
{me.Vector2d} anchorPoint - The current frame's anchor point (0,0) top-left to (1,1) bottom-right.

If you're creating a custom AnimationSheet, you can pass an anchorPoint in addition to the framewidth and frameheight.

var animationSheet = new me.AnimationSheet(0, 0, {
image : "animationsheet",
framewidth : 64,
frameheight : 64,
anchorPoint : new me.Vector2d(0.5, 0.5)
});

If you're using TexturePacker, frame anchorPoints are created automatically based on the "sourceSize", "spriteSourceSize", and "pivot" properties of the frame (assuming they all exist).

If an anchorPoint is set by the texture atlas json or manually like above, the entity.renderable's anchorPoint is drawn at the entity.body's anchorPoint. (as described in issue 715)

Otherwise, it acts the same as it does today, where the entity attempts to keep the renderable inside the body's bounds.

[obiot]

we don't really need a hasTextureAnchorPoint boolean, we can just check if anchorPoint is defined or not ?

[Giwayume]

Yes, but there was a reason I put that in there... hmm let me remember.

[obiot]

note that i'm not saying that we should remove it, just wondering if it's really necessary, as the less properties we have the better, but in the mean time it's not like this one is anyway impacting performances of the object.

[Giwayume]

OH! I see. I'm not crazy. The AnimationSheet inherits an anchorPoint from the Renderable class. The boolean is there so we know whether the anchorPoint came from the texture, or whether it was the default anchorPoint. If it's from the texture, the sprite's position is drawn differently than it is today (reference entity.js)

[Giwayume]

The boolean doesn't have to exist inside the atlas, but it does need to be defined when we get to the setAnimationFrame() function inside the AnimationSheet class.

[obiot]

Hi @Giwayume great submission! let me have a quick review and see if I ahve any questions on my side :P

[obiot]

wonder if there is any way to optimize this and get rid of the if/else statement. maybe @parasyte wil have some idea about it. Furthermore we had this #309 ticket, that maybe could be also integrated somehow through this changes you did?

[agmcleod]

Could dry it up by setting values to the return of the bits on lines 284, 287, 294, 297. Then do the repetitive x = ~~(0.5 + this.pos.x + this.body.x + xAnchor). that bit could even be in a method. Dynamically pass the property you want. x or y.

[Giwayume]

I think #309 is covered here, however the positioning is handled inside of the Entity instead of the Sprite class.

[obiot]

it seems to be yes (although me.Sprite should be updated as well to then comply with the new behavior here), but to be honest I did not have the time yet to go through all your changes :P

[Giwayume]

If the anchorPoint translation moves to me.Sprite, it would be difficult for me.Entity to keep its current positioning behavior, where it completely ignores the me.Sprite anchorPoint and tries to position the edges of the sprite based on body width/height.

I wasn't sure if it was OK to toss out the old positioning behavior. Some people may find it useful. Though, I think if we have both it makes the engine a little more confusing to use rather than committing to the new method. And having the Sprite positioned relative to Entity's body differently based on whether or not the AnimationSheet has an anchorPoint defined is also a confusing thing to learn.

But if the old positioning is tossed out completely examples will have to be re-written.

[obiot]

I would be ok to toss out the old behavior to be honest in the context of a major 3.0.0 release, it was not an optimal and perfect solution anyway, and #309 was aiming at fixing that.

let's wait for @parasyte feedback on this if you are ok :P

[Giwayume]

If we commit to the new method, the this.hasTextureAnchorPoint boolean has no purpose and can be taken out of AnimationSheet. It's only there to switch between new and old positioning method at the moment. The Texture frame's anchorPoint would still need to be checked if it exists or not, unless it's defaulted in the Texture class.

[Giwayume]

I have been talking to the owners of TexturePacker. There are some details with how the pivot changes when cropping vs trimming that I didn't include in the code. Will update.

EDIT: Nevermind. No coding changes are needed. The sourceSize variable gets updated by TexturePacker when cropping vs trimming.

[agmcleod]

Question worth asking. Does Shoebox leverage the original code as well? Will this break the shoebox support?

[Giwayume]

No, it only adds an anchorPoint if the supporting variables exist. Worst-case scenario for shoebox is they don't get an anchorPoint at the moment and it defaults to current functionality.

I've briefly looked at the shoebox export script and it seems most of the variables are the same if not exactly the same, but I haven't tested it because I didn't want to bother trying to set up legacy Adobe Air on Linux at the time.

[Giwayume]

I have tested the shoebox export script. All of the variables are the same as TexturePacker, except the pivot point is missing. So if you export from shoebox the anchorPoint will not be defined in the texture, unless we provide a way to override it.

It's looking more like even if the old positioning method is kept, there should be a "positioning method" flag in the entity itself. It shouldn't be based on whether or not the texture frame has a custom anchor point. The anchorPoint is already defaulted to [0.5,0.5] in the renderable super class of the AnimationSheet, so it's not like having a custom anchorPoint missing from the texture frame definition is a big deal.

[parasyte]

I like that idea! It doesn't seem really useful to have a boolean flag that determines whether to use the anchorPoint. In concept, it's ideal to remove conditional feature support by setting sane defaults when things are missing. I'll run through the patch and let you know if I have any comments. :)

[parasyte]

Don't forget to add the doc comment for the new settings.anchorPoint parameter. Just copy-paste this line for the formatting.

[Giwayume]

OK. I'm going to update and move the Sprite anchorPoint positioning logic into the Sprite class itself.

[parasyte]

That works, too!

[Giwayume]

If I draw a Sprite with its anchorPoint at its x and y pos, the debugPanel draws the bounding box incorrectly because it's using this.left (this.pos.x) and this.top (this.pos.y) as the starting point for the bounding rectangle. [debugPanel.js line 200]

Should I override this.left and this.top to take the pixel offset from the anchorPoint into account? I'm not aware of Sprites being used for collision so maybe this isn't much of an issue.

[parasyte]

That's a good point. The debug panel is something I've struggled with in the past, as well. I would be cautious about changing the geometry methods (left getter et al.) to require information from the parent, like its anchorPoint. We don't have this requirement now, which allows things like inserting a sprite directly into the scene graph without a parent entity.

on the other hand the entity draw method just needs to translate to its own anchorPoint before drawing the child sprite. Then the sprite translates to its anchorPoint as normal. This translation step is clearly missing from the debug panel:

melonJS/plugins/debug/debugPanel.js

Line 200 in 64eeb99

renderer.strokeRect(this.left, this.top, this.width, this.height);

[Giwayume]

I'm talking about the Sprite's anchorPoint, not the Entity's (parent's) anchorPoint. The Sprite's left property would be adjusted based off the Sprite's anchorPoint.

Anyhow, when I modify the left and top to adjust based on the anchor point if it exists, a jasmine test for the droptarget entity fails. It seems the top,left,bottom,right values should be adjusted only for Sprites, not all Renderables.

Making up for this in the debug panel seems hacky to me.

[parasyte]

I'm familiar. ;)

Imagine how you might have to modify these lines of code to properly run within me.Sprite.draw:

// in this case, the entity's anchor point is in relation to the body
// draw the renderable's anchorPoint at the Entity's anchor point
x = ~~( x + (this.anchorPoint.x * this.body.width) - (this.renderable.anchorPoint.x * this.renderable.width));
y = ~~( y + (this.anchorPoint.y * this.body.height) - (this.renderable.anchorPoint.y * this.renderable.height));

Copied directly from your patch, above. :) The issue here is that correctly positioning the sprite relies on the sprite anchorPoint and the entity anchorPoint. Since the entity is already doing this translation, it makes sense that the debugPanel should do it, as well. After all, the debugPanel is just patching me.Entity.draw ... Does it make sense?

[Giwayume]

Yeah, those lines have changed since the Sprite is responsible for drawing itself relative to its own anchor point now. So the problem is not in me.Entity.draw, it's in me.Sprite.draw.

I'm almost done, you'll see what I'm doing in a couple of minutes.

[Giwayume]

Don't know why this note didn't collapse, but I've updated the doc comment.

[parasyte]

This one won't collapse because you haven't changed the line that it points to. It's expected. 😃 Having a look at the updates, now...

[Giwayume]

OK, so I've defaulted the Entity class to draw with the new positioning method. I've added a boolean to the Entity class drawRenderableInBounds to switch back to the old method.

Because it defaults to the new method, I modified the texture.tps/json/png in the platformer example to set the pivot point at [0.5,1] to match the anchorPoint defined at [0.5,1] in those entities. (modifying these files makes it look like I've touched an extra 250 lines of code =P)

The Sprite class now owns the logic for drawing its position based on its anchorPoint.

[parasyte]

Less is more: https://gist.github.com/parasyte/fa9664cc19824c4ff7b7

I added the same translation into the debugPanel, so the conditional geometry offsets can be removed. The translation is exactly the same as what you do here. It's identical to the method used to draw the hitboxes around the entity; debugPanel needs to do the same translation for those hitboxes as the entity does for its child renderable.

[Giwayume]

Alright, how do I apply your patch? Or do I just copy it over.

[Giwayume]

Nevermind, I just copied over the changes and re-committed.

[obiot]

wow that looks super good ! i'm really excited by this one :)

two comments I see before we merge this baby :

• we miss a nice entry in the changelog
• do we keep the old drawRenderableInBounds options ? i know that we have a history of breaking changes in the framework, but 1st these are changes we discussed since a long time (so they were wanted), and it's for the 3.0.0 release, a MAJOR one where we can have breaking changes. personally i'm for removing the backward compatible option (plus i hate having a if/else statement in the draw function)

[Giwayume]

If you don't think having the old method as an option is useful, I'll remove it.

There's also one more thing I wanted to look at, I'm not entirely sure if the "Crop - Keep Position" option in TexturePacker is working correctly. I know that the other two "Trim" and "Crop - Flush Position" are correct.

[Giwayume]

Do you think I should add the old positioning method as a plugin or nah?

[parasyte]

I'm happy to remove the optional backward compatibility flag. It should be backward compatible just by passing the body anchorPoint to the sprite constructor, right?

[Giwayume]

You made me think for a second, but yes I believe that actually works. Maybe that plugin I made is useless...

[Giwayume]

Still waiting on feedback from the TexturePacker team on how "Crop keep position" is supposed to work.

[parasyte]

Maybe "backward compatible" isn't the right word, but "to get the old behavior"!

Do you want to wait on the feedback before deciding whether to merge this?

[Giwayume]

Yes, the TexturePacker guys have been surprisingly fast to respond.

[Giwayume]

Omg they responded to my email like 6 hours ago and I didn't notice... there's definitely a few changes to be made.

[Giwayume]

I'm done with the fix.

[obiot]

great great work thank you very much @Giwayume , merging now and updating the changelog !