Fix local bounds problems. Swap bounds with localBounds to do the trick.

[ShukantPal]

Fix for #6355

By swapping the _bounds object with _localBounds, we are preventing calculateBounds from editing the world-bounds.

Also, I am syncing the changes in the _boundsID with the world-bounds updateID. This will prevent the world-bounds from getting recalculated in the next getBounds(true) if it was already update-to-date.

• Tests and/or benchmarks are included
• Documentation is changed or added
• Lint process passed (npm run lint)
• Tests passed (npm run test)

[ShukantPal]

Hmm, this doesn't solve the JSFiddle I presented. On inspection, the _bounds of the object are correct now, except the sprite is still not rendering correctly.

[ShukantPal]

The sprite still renders wrongly because Container will update the transform of its children too. The rendering process does not update transforms.

[codecov-io]

Codecov Report

Merging #6365 into dev will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #6365   +/-   ##
=======================================
  Coverage   78.54%   78.54%           
=======================================
  Files          57       57           
  Lines        2825     2825           
=======================================
  Hits         2219     2219           
  Misses        606      606

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69136d5...8a02882. Read the comment docs.

[ShukantPal]

@ivanpopelyshev @bigtimebuddy This fixes the problem!

[ShukantPal]

I think I will do one more optimization tomorrow: on a container's children, we should swap the transform with a temporary transform instance. Then we won't have to call update-transform again after getLocalBounds.

[bigtimebuddy]

It is important if you could capture this modified bounds behavior in a unit test, preferably one that breaks on dev. That’s the best way to ensure that we don’t have a regression.

A good unit test uses only public APIs preferably and considers all the scenarios in which it could be used, including edge cases.

[ivanpopelyshev]

As I said in the issue:

1. I believe both of getLocalBounds in Container and DisplayObject should have _ version of methods because we have overrides in Sprite and there might be an override in spine library
2. I'm sure I can make a test case when this thing fails. I'll work on it in weekend
3. Try integrate my idea with axis-alighned fallback. However, with right architecture i can integrate it later.

[ShukantPal]

@ivanpopelyshev Your idea could be added in another PR - when it becomes your priority.

[ShukantPal]

Overrides will not be affected at all. This should be a backward compatible change after I swap the child transforms too.

[ShukantPal]

@ivanpopelyshev I’m looking for your tests today that should fail with this change.

[ivanpopelyshev]

OK, I wanted to merge dev into it but made something wrong. Can you please re-do it and push with force?

[ivanpopelyshev]

OK, never mind, I've rebased it myself.

[ivanpopelyshev]

OK, now I understand how good is that PR:

1. It is fixing stupid issue.
2. It does not gives us a new fundamental way to store things, it just fixes current problem.
3. I like it that _localBounds makes sure that subsequent call (c.height after c.width) does not trigger re-updates.

What to change:

1. We need tests. I'll help to convert jsfiddle to test and maybe make a test that width/height check does not trigger update two times.
2. _localBounds is one more object. Maybe make it null by default.

Good job!

[ivanpopelyshev]

We have to update all objects (Sprites, e.t.c.) getLocalBounds to pass that param.

@bigtimebuddy we have a problem that getBounds() has first argument bool and second Rectangle, and for getLocalBounds its rectangle and then bool :) The heck we do in this case? I propose we add skipChildrenUpdate here as the first param. That way both methods will have the same signature and false, null by default. Maybe add deprecation?

[ShukantPal]

@ivanpopelyshev Please recheck the standing of the changes.

[eXponenta]

@ivanpopelyshev Please recheck the standing of the changes.

Doesn't work at all in real cases =)

DEV:
https://www.pixiplayground.com/#/edit/5uTdn6fP5w-DJrLXKuPWL

Your:
https://www.pixiplayground.com/#/edit/Qyl4KFWghAu3AwXfRLOSV

[ShukantPal]

@eXponenta I already know. Review the types of changes first and then I’ll fix the bugs today.

[eXponenta]

@eXponenta I already know. Review the types of changes first and then I’ll fix the bugs today.

How should we review it when it doesn't works properly?

[ShukantPal]

@eXponenta Sometimes people don’t like me changing something even if it works

[ivanpopelyshev]

Don't worry, we'll take care of it in weekend together.

[ShukantPal]

@ivanpopelyshev Any updates?

[ivanpopelyshev]

Cant commit, one test is failing because I added extra condition: two consecutive getLocalBounds calls shoould not trigger second calculateBounds.

[ShukantPal]

@ivanpopelshev Will look at it in evening

[ivanpopelyshev]

Yeah, if you manage to adjust boundsID hacks that way tests passes - it will be awesome!

[cursedcoder]

Looks good, but tests.

[ivanpopelyshev]

My fault - I made extra test that I thought can be passed with this PR, but it turns out something is wrong with boundsID's. I'm waiting for @SukantPal when he will be free to investigate whether i made a mistake or he did

[ShukantPal]

@ivanpopelyshev I pushed changes to make the tests to pass. I following breaking changes will occur:

• Display objects that have a change in their natural bounds will have to declare that by updating _boundsID. They will not get bounds update for free as before (because updateTransform will not be called if bounds are already update-to-date in getLocalBounds).

---

• Graphics.clear() updates _boundsID.

[bigtimebuddy]

Could you help me understand the breaking change more? If you could show some code of what would change, that’d be helpful.

[ShukantPal]

@bigtimebuddy I'll explain the "breaking" change a bit rigorously as follows:

Let's say we have a display-object object; it is (and was) expected that if its transform has not changed and object._boundsID is not dirty thenobject.getBounds() will calculate the same bounds.

Now, the local bounds are, by definition, defined as bounds without any transforms applied. That leaves us with: if object._boundsID is not marked dirty, then object.getLocalBounds() will calculate the same value.

---

Previously, getLocalBounds always deferred to getBounds - which always did an updateTransform. Now, updateTransform marks _boundsID dirty, well because it thinks that the transform has changed.

Hence, since _boundsID was made dirty internally in getLocalBounds - that meant calculateBounds was called each and every time getLocalBounds was called, and that authors of display-objects (like PIXI.Graphics#clear()) did not have to worry about marking their bounds dirty.

This assumption is now incorrect with this PR. If object._boundsID is in-sync with this._bounds.updateID, then the local bounds won't be updated.

---

Why?

By not updating the local bounds when they aren't marked dirty, we prevent unnecessary bounds calculations and transform updates.

Future work

Similarly, we could prevent getBounds() updates if the transform itself has not changed after a updateTransform. That would be done in a different PR.

Another use-case of this PR

When the scene graph is being rendered, it is expected that this._bounds are update-to-date. However, calling getLocalBounds() previously changed what getBounds(true) returned. This is because getLocalBounds() would store its result in this._bounds and getBounds(true) wouldn't update _bounds (no transform update). This results in many errors throughout PixiJS (because the bounds returned by the node are incorrect).

This PR swaps this._bounds with another this._localBounds. This way getLocalBounds will not affect the result of getBounds(true). Sadly though, it is still unable to prevent getLocalBounds from affecting the getBounds(true) value of grandchildren (and nodes below 1st generation children). I will work on a performant fix for that later on.

[ShukantPal]

@bigtimebuddy We have a problem with parent containers whose local bounds depend on the transform of children in the parent’a reference frame.

I will push a fix for that, if possible.

[bigtimebuddy]

@SukantPal, I think the work that you've done here is terrific. I commend you for optimizing these code paths related to caching bounds, because it's subtle, difficult work.

One of the challenges of PRs like this, is that I (and probably others on the PixiJS team), don't fully understand the extent to which this may produce unwelcome side-effects for developers. It's one thing add unit-tests, it's another to do a full integration where users heavily rely on bounds.

Our goal as an open source library is, most importantly, is to make sure we do no harm. I think it's important that you can work with developers (@themoonrat, @GoodBoyDigital, and others) to test this build into real-world, complex apps and games. As much as you're able to prove and show that this will not negatively impact users, it will be easier to approve.

[ShukantPal]

@bigtimebuddy I have removed the sync barrier (stopping local-bounds recalculations) so there are no side-effects. I think @ivanpopelyshev would be okay to merge this now. (So this PR now only prevents getLocalBounds from storing its result in this._bounds, preventing corruption this.getBounds(true), via the _localBounds swap).

To prevent unnecessary bounds updates, we will need to implement a system where transform updates are propagated up the scene graph. This will be experimented on another branch.

[ivanpopelyshev]

There's only one thing left - _localBounds is still created in constructor, and its triggered only by calling getlocalBounds of the same element - it better be a lazy variable.

[ivanpopelyshev]

Congratulations @SukantPal with solving the problem that has a trail of dead PR's for years. Now we've covered it with test and it wont be possible to regress.

Btw, I added one more test in my last commit, the one we discussed two days ago.

[GoodBoyDigital]

Hey @SukantPal , thanks for this! Going to test this branch on one of our more layout heavy games! Will let you know how it goes :D

[GoodBoyDigital]

works like a boss!