Fix for DisplayObject/DisplayObjectContainer - getting dimensions or bounds do NOT retrieve proper values #2639

Merged
merged 4 commits into from Jul 23, 2016

Projects

None yet

4 participants

@fmflame
Contributor
fmflame commented Jul 17, 2016 edited

The PR changes include:

  • TypeScript Defs
  • Nothing, it's a bug fix

I noticed that getting dimensions or bounds changed in Phaser 2.6.1 and it was bugged - returned dimensions were doubled in the direction of the scaling for example, but the nice thing was that this time the dimensions included the rotation of the object - which I think should be the default behavior. It also bugged getLocalBounds which started returning global getBounds();
When I checked the previous version of phaser 2.5.0 - total different story. getLocalBounds returns the object without any transformations to it. Getting width and height works ok, but rotation wasn't calculated to the dimensions at all.
In all cases only getBounds returned proper dimensions, which is obviously not enough and this is also a very important issue to be resolved as soon as possible since it's a major core component feature used all the time.
That's why I decided to spare the time and hopefully find a good fix for all of this mess and I think I finally got there and tried to make the changes as much as possible with your Code of Conduct. This is actually the first time I am requesting a pull so I hope it will do some good. I did compare my results with how Flash handles dimensions - they match, so I do hope I didn't miss something and believe you guys would make sure everything works ok.
To help you with reviewing and possibly (and hopefully) accepting this pull request I've made available online 3 demos of the issues and with demo of the fix of course - check the console log:

  1. http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemo_v2.5.0/
  2. http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemo_v2.6.1/
  3. http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemoFixed_v2.6.1/

Here is a zip file of the demo projects (build with VS2015):
http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemos.zip

I really hope to see this FIX in the next version - would help me and a lot of people I guess to get the job done properly!

Thank you!

@fmflame fmflame Fix for DisplayObject/DisplayObjectContainer - getting dimensions or …
…bounds do NOT retrieve proper values

* Documentation
* TypeScript Defs
* Nothing, it's a bug fix

Describe the changes below:
I noticed that getting dimensions or bounds changed in Phaser 2.6.1 and
it was bugged - returned dimensions were doubled in the direction of the
scaling for example, but the nice thing was that this time the
dimensions included the rotation of the object - which I think should be
the default behavior. It also bugged getLocalBounds which started
returning global getBounds();
When I checked the previous version of phaser 2.5.0 - total different
story. getLocalBounds returns the object without any transformations to
it. Getting width and height works ok, but rotation wasn't calculated to
the dimensions at all.
In all cases only getBounds returned proper dimensions, which is
obviously not enough and this is also a very important issue to be
resolved as soon as possible since it's a major core component feature
used all the time.
That's why I decided to spare the time and hopefully find a good fix for
all of this mess and I think I finally got there and tried to make the
changes as much as possible with your Code of Conduct. This is actually
the first time I am requesting a pull so I hope it will do some good. I
did compare my results with how Flash handles dimensions - they match,
so I do hope I didn't miss something and believe you guys would make
sure everything works ok.
To help you with reviewing and possibly (and hopefully) accepting this
pull request I've made available online 3 demos of the issues and with
demo of the fix of course:
1.
http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemo_v2.5.0/
2.
http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemo_v2.6.1/
3.
http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemoFixed_v2.6.1/

Here is a zip file of the demo projects (build with VS2015):

http://www.nedyalkov.net/filip/demos/phaser/IncorrectDisplayObjectDimensionsDemos.zip

I really hope to see this FIX in the next version - would help me and a
lot of people I guess to get the job done properly!

Thank you!
483a3b6
@fmflame
Contributor
fmflame commented Jul 18, 2016 edited

If this fix is accepted then I guess the only thing left to change is in the docs the DisplayObjectContainer.getBounds() method that I've changed to getBounds(targetCoordinateSpace) to make it more flexible and to allow checking bounds to any target not just local and global.
Sorry I haven't included the doc change in this commit - didn't think about it at the time.
Also having said that - with this fix - I think Phaser.Group bottom top etc could get their value from getLocalBounds() instead of getBounds() and then it wouldn't be a problem if the group is added to a parent with transforms. This just came up to me so I am not sure if I am right, but please let me know what you think. Thanks!

@st0nerhat
Contributor

This is likely related to #2627

@fmflame
Contributor
fmflame commented Jul 18, 2016

@st0nerhat yes, sorry, I didn't know how to properly mention those two commits are for the same issue. I do hope I found the proper fix for it!

@fmflame
Contributor
fmflame commented Jul 19, 2016

I mentioned that DisplayObjectContainer.getBounds(targetCoordinateSpace) allows checking bounds to any target not just local and global, but actually right now it doesn't. What it can do is:

  1. if targetCoordinateSpace is the same instance as the caller of getBounds(), then it will return the bounds of the caller without any transformations;
  2. if targetCoordinateSpace is any valid DisplayObject it will return the local bounds of the caller.
  3. if targetCoordinateSpace is null/undefined it will return the global bounds of the caller.

The idea was that it will return bounds related to any targetCoordinateSpace, parent or no parent. Well it still fixes getting 1) 2) and 3). Still good enough, I was hoping it will do more than that but it needs to be reworked a bit so it could do the trick. If I have the time I will add that too or you guys could finish it if you'd like. But in any ways even if you add this fix as is, it will still be a lot better than what it is or what it was before, because it will fix the current issues we have with dimensions.

@fmflame
Contributor
fmflame commented Jul 19, 2016

I am almost done with a completely fixed version for this so I will probably close this commit and create a new one later today.

@photonstorm
Owner

Go for it. I'm away today so can't merge anything until tomorrow anyway,
and this PR looks great so it'd be good to get it right.

On Tuesday, 19 July 2016, Filip Nedyalkov notifications@github.com wrote:

I am almost done with a completely fixed version for this so I will
probably close this commit and create a new one later today.


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

Photon Storm Ltd.
http://www.photonstorm.com

Skype: richard.davey
Twitter: @photonstorm

Registered in England and Wales.
Company ID: 8036404
VAT Number: 136 4333 27

@fmflame
Contributor
fmflame commented Jul 20, 2016

I will create the fixed pull request very soon :) Sounds great you like it :)

@fmflame
Contributor
fmflame commented Jul 20, 2016

I am done with the fix. I think it works great. I did a lot of testing and thinking of how is the best way to make it work that makes sense. I hope you'll like it. I need a few more hours - I still need to redo a little bit the demos and write a description of the changes that makes sense. So until later ;)

@photonstorm
Owner

Don't worry, there's no rush. Spend time to get it right. 2.7 won't be released for at least a week, so you've plenty of time.

fmflame added some commits Jul 20, 2016
@fmflame fmflame Fix for PIXI's DisplayObject/DisplayObjectContainer - getting correct…
… dimensions and bounds

With the previous fix what the getBounds did was:
1) if targetCoordinateSpace is the same instance as the caller of
getBounds(), then it will return the bounds of the caller without any
transformations;
2) if targetCoordinateSpace is null/undefined it will return the global
bounds of the caller.
3) if targetCoordinateSpace is any valid DisplayObject it will return
the local bounds of the caller.

What this fix does is fixing 3) along with other obsolete code that
wasn't necessary so I reverted it.
So now if the targetCoordinateSpace is a valid DisplayObject:
- if it's a parent of the caller at some level it will return the bounds
relative to it
- if it's not parenting the caller at all it will get global bounds of
it and the caller and will calculate the x and y bounds of the caller
relative to the targetCoordinateSpace  DisplayObject

Also I have fixed how empty groups are treated when they have no other
children except groups, so now calculations are correct. They obviously
have 0 width and height but are still being positioned and other things
could possibly relate to that bounds and it didn't make sense to me to
ignore them.

Also added a DisplayObjectContainer.contains(child) method which
determines whether the specified display object is a child of the
DisplayObjectContainer instance or the instance itself. This method is
used in the new getBounds function.

Corrected DisplayObject's default _bounds rect from (0, 0, 1, 1), to (0,
0, 0, 0) - it doesn't seem to break anything and also in the getBounds
before the fix, when there were no children it assigned a (0, 0, 0, 0)
rectangle to it so I am pretty sure it's safe to correct it.
4f21e70
@fmflame fmflame Just removed some whitespace I don't remember adding it... 7206453
@fmflame fmflame Fixed formatting to match the general formatting of the code d18f303
@fmflame
Contributor
fmflame commented Jul 21, 2016

Hi I've committed the changes to the branch on my forked rep so I see they automatically translated here - so I won't close this pull request, because It does seem it will do the job. I have updated all links with the demos so you could check them out. I really hope I didn't miss something this time. I hope I helped out and will be happy to see this in the next version :)

You can read the full description of what changed and how from the initial commit in the second commit's description.

Regards

@photonstorm photonstorm merged commit fb1a7f1 into photonstorm:dev Jul 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@photonstorm
Owner

Thanks for opening this issue, and for submitting a PR to fix it. We have merged your PR into the dev branch, and attributed the work to you in the README. If you need to tweak the code for whatever reason, please submit a new PR.

@fmflame
Contributor
fmflame commented Jul 25, 2016

Awesome! :) That's great guys. Thank you!

@namark

@fmflame @photonstorm If I'm not mistaken getLocalBounds used to return object bounds not accounting for the parent, which would be return this.getBounds(this);. Is this an intentional change?... Causes some annoying issues in my project...

Owner

True, surely 'local' bounds are just that - what do you reckon @fmflame ?

Contributor
fmflame replied Jul 27, 2016 edited

@namark yes getLocalBounds should not account for parent transformations and I believe this is how it works right now. If you look at what happens in this.getBounds(this.parent); with this.parent, you will notice that the parent gets a temporary blank matrix (PIXI.identityMatrix) and then we are updating the transform of its children. This is not seen here cause I think we are commenting on an outdated commit - there are more fixes in the next ones - so check them out to see what I am talking about. Plus I just posted an additional changes today. Everything should be working perfectly for DisplayObject/DisplayObjectContainer. And for Sprite, Strip and Graphics - it should work the same as before with the latest fix - I do think it is not working as it should and that there are issues with them but they are from before and I don't have the time to fix them at this point in time.
Anyway, if you are having issues it is very possible that with the new fixes old calculations you did would be invalidated and you should rewrite since they weren't correct in the first place. But what you are asking - does work as you say. There is difference in how the code gets bounds and updates transforms. I have provided examples and I tripple checked with similar libraries of how getting bounds should work so I doubt there is a mistake at this point.
this.getBounds(this) -> should return the untransformed bounds of the DisplayObject for e.g if you have a graphic (0, 0, 200, 100) with scale = 2, it would return (0, 0, 200, 100). If you want to get the bounds of the object with it's transforms then you would call this.getBounds(this.parent) and the result would be (0, 0, 400, 200), which is the local bounds of the DisplayObject - meaning it's the bounds it has in it's local coordinate, which is in his parent's coordinate, including it's own (not parent's) transforms.
So I do believe what I did is the correct way of how this should work. Please check if you have the latest fixes pulled @photonstorm already merged them all and it's possible you'd have to updated your logic to work with the changes. If you are having further questions or doubts if things should work the way I changed them - please provide me with some examples and logic/reasoning behind them of what you'd expect. You can also check the examples I've provided in the PR description - IncorrectDisplayObjectDimensionsDemoFixed_v2.6.1 is updated with the latest changes and fixes for this.

@fmflame I am on the latest dev branch, and this is the relevant change for getLocalBounds method, I believe.

What I meant is that this "this.getBounds(this) -> should return the untransformed bounds" is exactly what getLocalBounds used to do, and I don't see anything inherently wrong with that. So what if other libraries do it differently? Logically I see no reason why, say, the object's position in its parent, should in anyway affect its "local" bounds.

But even if you are willing to introduce this change, the way things are now, since you effectively reverted the method for some objects in a later commit, there is a major inconsistency. Just create an Image object and observer the local bounds(meaning what getLocalBounds returns). No matter how you transform it (position/scale/rotate) the local bound remain the same. Do the same with, say, BitmapText, and local bounds will change with the transform. If nothing else this would be a nightmare to document. This is not how things used to be, and for me, frankly, it is easier to adjust a single line of code in my fork, instead of restructuring my entire project... just thought to point this out...

Owner
photonstorm replied Jul 27, 2016 edited

Hi @fmflame @namark - please can we stop using the commit comments to discuss this, it's too important. I have created a new issue for it #2667 and copied the above in, so please both let's continue it there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment