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

[Closes #618] Fix Actor anchoring and scaling draw #619

Merged
merged 11 commits into from
Aug 18, 2016

Conversation

kamranayub
Copy link
Member

Closes #618

I added a new visual test for testing anchoring with fillRect and Sprite drawing. It's a mess in the current version, as you can see:

Yuck

The anchor is being double-scaled and sprite drawing is super weird even with setCenterDrawing to false for everything except the center test.

After making some changes to the draw call logic, I believe I've fixed it properly:

image

I removed all the center drawing cruft, since now it will properly draw anchored correctly.

Proposed Changes:

  • Simplify Actor.draw call logic to avoid double scaling
  • Remove need for setCenterDrawing logic

@kamranayub kamranayub added the bug This issue describes undesirable, incorrect, or unexpected behavior label Aug 4, 2016
@kamranayub kamranayub added this to the 0.7.0 Release milestone Aug 4, 2016
@kamranayub
Copy link
Member Author

There's still some work left for children/sprites.

@eonarheim
Copy link
Member

I think I've figured this out, let me explain.

The problem: Sprite centering only worked when the sprites were the exact dimensions of the actors bounding box. This is because the actors anchor is set to the middle (.5, .5) which is a good default I believe, so the default drawing are shifted actor.width * .5 and actor.height * .5 and because sprites draw from top left by default this works great. However, if these dimensions don't line up exactly, either the sprite is smaller or larger than the bounding box of the actor, the offsets are not correct.

The solution: If we want drawings to be centered, we need to account for this by developing an additional offset from the actor anchor itself. The formula below I believe accomplishes this in all cases.

The idea is that we want the sprite offset (the relative position in the drawing for example (.5, .5) is the center) to be translated to the center of the actor.

image

@eonarheim
Copy link
Member

Having a spriteOffset of (.5, .5) has a natural consequence of centering sprite scaling around the center of the actor, so if you want a top left to be the center of scaling you must also set spriteOffset to (0, 0) as seen here:

image

This begs the question, should spriteOffset and anchor always be the same thing?

@eonarheim
Copy link
Member

@excaliburjs/core-contributers It feels like the answer should be yes, spriteOffset and actor.anchor should always be the same thing

The sandbox platformer looks and behaves like it should this way
image

@eonarheim
Copy link
Member

@kamranayub spotted an issue in the coordinate test, child actors were not centering on the grid based on their anchors

image

The fix in 62ff021 was to wrap the anchor translations in a ctx.save() and ctx.restore() to localize the transforms due to anchoring, and let the other transforms propagate to the children
image

@eonarheim
Copy link
Member

@excaliburjs/core-contributers if we are in agreement this sucker can be merged!

@kamranayub
Copy link
Member Author

I'm so ready to merge this, let's do it!

On Thu, Aug 18, 2016 at 12:30 PM Erik Onarheim [email protected]
wrote:

@excaliburjs/core-contributers
https://github.com/orgs/excaliburjs/teams/core-contributers if we are
in agreement this sucker can be merged!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#619 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAiaayGP4Rm20bccrrU4yToJPBOTPOyzks5qhJawgaJpZM4JcVW4
.

@eonarheim eonarheim merged commit db9bca2 into master Aug 18, 2016
@eonarheim eonarheim deleted the 618-anchor-scaling branch August 18, 2016 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Center anchored actors are not drawn at correct canvas coords when scaled
2 participants