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

Entity anchor point is reversed compared to me.Sprite (renamed) #834

Closed
krojew opened this issue Nov 14, 2016 · 24 comments
Closed

Entity anchor point is reversed compared to me.Sprite (renamed) #834

krojew opened this issue Nov 14, 2016 · 24 comments
Milestone

Comments

@krojew
Copy link
Contributor

krojew commented Nov 14, 2016

When passing an image to Entity constructor, a child renderable gets created. This results in a strange behavior - the Entity has it's own anchor point, but also the child has a separate anchor point. This leads to situations where setting the point to 0, 0 on the entity causes it to be displayed half of image offset. This is very confusing and also can be related to #833

@krojew
Copy link
Contributor Author

krojew commented Nov 14, 2016

Also, the anchor point is inconsistently applied. Entity applies a positive offset based on the point, while Renderable applies a negative offset.

@agmcleod
Copy link
Collaborator

Yep, I'd be curious to see if @obiot has a reason for this. While confusing, maybe it's correct? lol. It was done along with the transform changes for melonjs 4. The idea was to keep feature/API parity, but I wouldnt be surprised if a bug or two cropped up.

Here's the subtract on sprite: https://github.com/melonjs/melonJS/blob/master/src/renderable/sprite.js#L547

The addition on entity: https://github.com/melonjs/melonJS/blob/master/src/entity/entity.js#L287

@obiot
Copy link
Member

obiot commented Nov 14, 2016

I did not change the "behavior" of the anchor points, or at least not intentionally, but yes we have to admit that our current design/usage could be improved (not to say fucked up).

Personnally i only use the entity anchor point :
https://github.com/melonjs/melonJS/blob/master/examples/platformer/js/entities/player.js#L59
works perfectly so far in all my projects

We had the plan to redesign the entity object, and adress this in the mean time, but this got delayed somehow... but if anyone has any good proposition(s) on how we could better manage these anchor point, it would help !

@agmcleod
Copy link
Collaborator

Ah yes sorry, i was more basing on when the code was put there as per the latest commit :)

@obiot obiot changed the title Double anchor point for entity with image Entity anchor point is reversed compared to me.Sprite (renamed) Feb 28, 2017
@obiot obiot added this to the 5.0.0 milestone Feb 28, 2017
@obiot
Copy link
Member

obiot commented Feb 28, 2017

small up for this one, as indeed the anchor implementation between Entities and other renderable is not consistent and it's time to clean-it up if we also want to fix related bounds issue (#833, #848) in a clean way :
Entity : https://github.com/melonjs/melonJS/blob/master/src/entity/entity.js#L277-L279
Renderable : https://github.com/melonjs/melonJS/blob/master/src/renderable/renderable.js#L377

Not to mention that me.Entity does not follow our documentation :
http://melonjs.github.io/melonJS/docs/me.Renderable.html#anchorPoint

@obiot
Copy link
Member

obiot commented Feb 28, 2017

@parasyte @agmcleod just to be sure we all agree, the correct one we want is the Renderable way, right (negative offset, that is consistent with the documentation) ?

@parasyte
Copy link
Collaborator

Speaking of which... It looks like this line belongs in postDraw()

@obiot
Copy link
Member

obiot commented Feb 28, 2017

indeed, but that's a cheap trick to avoid calculating ax and ay in the postDraw method, but you are right it should be there, else if the end user wants to also do something with currentTransform in it's own draw method, the transform will be off.

I had the plan to use an ObservableVector for the anchor point (to manage the bounds issue), so I might actually do it now and pre-compute the offset, so that I can reuse it, and it even then save a few cycle for every single renderable.

@parasyte
Copy link
Collaborator

They can be stored in private state variables. I really like the WeakMap setup for private state, but worst case scenario, an extra property on the class isn't terrible.

@obiot
Copy link
Member

obiot commented Feb 28, 2017

I just realised that this is more complex than I thought, because it also needs to then be taken in account when the object bounds size is updated!

any comment else on this :

@parasyte @agmcleod just to be sure we all agree, the correct one we want is the Renderable way, right (negative offset, that is consistent with the documentation) ?

@obiot
Copy link
Member

obiot commented Mar 1, 2017

@parasyte @agmcleod call me crazy, but looking at this with a well rested eye (i've been travelling too much recently, and that other ticket #854 drove me crazy for a couple of weeks), i just realized that me.Renderable is the wrong one and me.Entity is correct !

based on the documentation we have, an anchor point of [0, 0] corresponds to the top-left part of the image, and an anchor point of [1, 1] the right-bottom part. So me.Entity is correct, but the base me.Renderable object is doing the opposite.

Now, this is the opposite of what is doing other frameworks, but we actually already agreed on that.

am I correct ? i would appreciate some comment so that we can finally close this one :)

@obiot
Copy link
Member

obiot commented Mar 1, 2017

err.. changing this is leading to crazy result, with both anchor point combined.... i'm really not sure what I/we should do here actually.

@parasyte
Copy link
Collaborator

parasyte commented Mar 1, 2017

I haven't had a chance to review anything in this ticket. From the initial report to the finding that one-or-the-other things is apparently wrong ... I won't have any more info for you in the short term. :(

My only excuse is that I have only slept for a few hours each night for the past week, and spent a few 22-hour days at work; we had a massive service outage starting Thursday morning and ending Saturday afternoon. We had a second service disruption for about 4 hours today during the AWS outages. And a nice little bug in our scheduler topped off the night by slowly leaking disk allocations/reservations, causing one of our capacity alarms to trigger!

Anyway, I'm beat. I'll be happy to look into this after I'm rested and can start to think straight.

@obiot
Copy link
Member

obiot commented Mar 1, 2017

ouch... sorry to read that my friend ! Have a welllllll deserved rest then and talk to you later !

@agmcleod
Copy link
Collaborator

agmcleod commented Mar 1, 2017

So I think the original point about 0,0 being object center tends to work if we were using a more standard 3d/opengl coordinate system, instead of pixel screen coordinates. Where 0,0 is the center of the object, Y positive is up instead of down. I think we should keep it consistent with the way the world coordinates work, to help avoid confusion.

@parasyte
Copy link
Collaborator

parasyte commented Mar 1, 2017

From my POV, origin-center vs origin-top-left has nothing to do with the direction of the y-coordinate or even that we use unit spaces that matches a pixel grid.

AFAICT, the issues raised here are inconsistencies with the anchor point direction (both coordinates?) on one of the two objects, but it is unclear which ... even with a different origin location, one of these objects would still be opposite. There might be a good reason for that, or it might just be a confusing artifact of the design. Or just a bug that crept in without test coverage. Who knows? I still haven't looked. Just woke up, and time for work again! :)

@obiot
Copy link
Member

obiot commented Mar 2, 2017

indeed, the issue i was referring to is not that much about our coordinates system, but rather on the fact that the anchor point direction is different between me.Entity and the base me.Renderable object.

me.Renderable subtracts the width or height multiplied by the corresponding anchor point value, and me.Entity adds them.

@obiot
Copy link
Member

obiot commented Apr 6, 2017

@agmcleod @parasyte
hi guys,

I know that you are busy on other things (me too, as if you noticed I did not make a single commits since a couple of weeks), but I need your help with this last one, just to be sure we are all aligned and agreeing on this change.

the big change is on the entity anchor point, that is now aligned with how renderable normally use it :
bce96c9#diff-1089c7ebe64469651baf69f6982fa3de

this leads to some funky scenario with the platformer, like with the enemy entity, where I need now to use negative value to align them :
bce96c9#diff-494ace7c0257e21acb01de660bcce1fb

really really appreciate your help here, so that we can finally close all these related anchor tickets we have (once this one done, all the other will fall in line)

@parasyte
Copy link
Collaborator

parasyte commented Apr 6, 2017

@obiot My only feedback is that this change is surprising. The renderable anchorpoint needs to be changed? IIUC, the anchorpoints should default to [0.5, 0.5], so the renderable should just be centered to the entity. Assuming that both are the same size, the positions would inherently be equivalent. Under that assumption, I don't understand why this change was made or what it's actually accomplishing.

@obiot
Copy link
Member

obiot commented Apr 6, 2017

lol....

Renderable : https://github.com/melonjs/melonJS/blob/master/src/renderable/renderable.js#L368-L377
Entity : https://github.com/melonjs/melonJS/blob/master/src/entity/entity.js#L277-L280

do you see any differences ? that's what changed

there is absolutely no changes to the default anchor point value, still 0.5

@parasyte
Copy link
Collaborator

parasyte commented Apr 16, 2017

No, it still doesn't make sense. The change to the coin entity suggests that the coin will be drawn with the sprite's upper-left corner anchored to the entity's center;

  • entity anchorPoint = <0.5, 0.5>
  • renderable anchorPoint = <0.0, 0.0>

Unless I'm mistaken, this is how I'm reading the anchorPoint states for that coin entity. My confusion is from trying to understand how these values lead to the correct behavior.

  1. renderer is translated to the entity/body position.
    • The renderer is pointing at the upper-left corner of the entity.
  2. renderer is translated to the entity's anchorPoint.
    • The renderer is now pointing at the center of the entity.
  3. The renderable draw has two different paths, depending on whether the transform has been set. Regardless, the outcome is the same:
    1. renderer is translated away from the renderable by its anchorPoint.
    2. renderer is translated away from the renderable by its anchorpoint.
      • The renderer hasn't moved, because the renderable anchorPoint is zero.

After all that, the coin sprite is drawn with its upper-left corner at the center of the entity? If the visuals are correct after all this, then that means there is something else inserting an offset somewhere which is what the zero anchorPoint is trying to cancel.

The default behavior with renderable anchorPoint at its center, step 3.i (or 3.ii) cancels step 2, which puts the upper-left corner of the renderable at the upper-left corner of the entity. This is the expected behavior.

That's why it surprises me, but I could be completely misunderstanding what's actually happening. If you have any insights, please share.

@obiot
Copy link
Member

obiot commented Apr 19, 2017

this conversation is getting nowhere, we keep not understanding each other. All i'm saying is that anchor point are translated to different-opposite direction between entity and renderable. And from the initial discussion this was what we were trying to fix.

once again (assuming values are different than 0, 0) :

if now this is the desired behavior, that fine, i give up anyway. But I still don't understand why then all these discussions about the anchor point being inconsistently applied between different object type (me.Entity vs me.Renderable)

@parasyte
Copy link
Collaborator

I'm familiar. The "opposite" translation is necessary for entity <-> renderable behavior, but is also the reason for confusion in this very ticket. Whether this behavior is "expected" is up for debate... But that's not my question.

Given that it is the current behavior, I'm questioning how the provided values (<0.5, 0.5> and <0.0, 0.0>) are operating with correct results. Because logically these would have vastly incorrect results.

I guess it's more a question of lacking unit tests that show expected vs actual behavior.

@obiot
Copy link
Member

obiot commented Jul 22, 2020

see #1008

@obiot obiot closed this as completed Jul 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants