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

Make core independent from display #6450

Merged
merged 7 commits into from
Jun 18, 2020
Merged

Make core independent from display #6450

merged 7 commits into from
Jun 18, 2020

Conversation

ivanpopelyshev
Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev commented Mar 1, 2020

Continuation of #6443

@pixi/core can be independent from @pixi/display !

_lastObjectRendered can be null after that PR, so I patched InteractionManager too.

For now I decided that its ok to have temp parent separate for each possible detached element.

I also couldn't built pixi because of strange DisplacementFilter import, I dont know if you guys want to resolve it in one of other PR's

@ivanpopelyshev ivanpopelyshev changed the title make core independent from display Make core independent from display Mar 1, 2020
Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Also, can you update the core package.json to change the dependencies?

@ivanpopelyshev
Copy link
Collaborator Author

Many times people were trying to change that thing. This PR is a delcaration that temp display object stays, but at least we can refactor it so it wont haunt us.

@codecov-io
Copy link

codecov-io commented Mar 1, 2020

Codecov Report

Merging #6450 into dev will increase coverage by 1.60%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #6450      +/-   ##
==========================================
+ Coverage   78.54%   80.15%   +1.60%     
==========================================
  Files          57       57              
  Lines        2825     2827       +2     
==========================================
+ Hits         2219     2266      +47     
+ Misses        606      561      -45     
Impacted Files Coverage Δ
packages/mixin-cache-as-bitmap/src/index.js 80.23% <0.00%> (+26.94%) ⬆️
...kages/canvas/canvas-renderer/src/CanvasRenderer.js 85.84% <100.00%> (-0.13%) ⬇️
packages/interaction/src/InteractionManager.js 94.33% <100.00%> (+0.03%) ⬆️

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 fd68c0b...faae01a. Read the comment docs.

Copy link
Member

@bigtimebuddy bigtimebuddy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One other thing, can you add test(s) for these new temp functions?

@ivanpopelyshev
Copy link
Collaborator Author

I don't know which non-trivial things to add there at the moment. I prefer not to make trivial tests.

@bigtimebuddy
Copy link
Member

You're reusing this pattern in three places, and I think we can simplify reuse:

const cacheParent = displayObject.enableTempParent();
displayObject.updateTransform();
displayObject.disableTempParent(cacheParent);

Maybe make a variant of updateTransform (safeUpdateTransform? parameter flag?) and remove enabledTempParent and disableTempParent methods.

@ivanpopelyshev
Copy link
Collaborator Author

ivanpopelyshev commented Mar 2, 2020

OK, added a test.

We declined adding a parameter before because of extra if making updateTransform ugly.

no, we cant do that because there might be other operations between updateTransform and disableTempParent.

Considering all the cases where i saw temp parent is needed, and all the possible cases i know (updateColorTransform and others) it will be better if people see those lines. We cant to hide them, they are important for understanding.

@@ -29,7 +29,6 @@
},
"dependencies": {
"@pixi/constants": "5.2.1",
"@pixi/display": "5.2.1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@cursedcoder
Copy link
Member

@ivanpopelyshev did you forget to push? Because I can't see any tests.

@ivanpopelyshev
Copy link
Collaborator Author

@ivanpopelyshev
Copy link
Collaborator Author

We have to wait for localBounds fix, then I'll go over this PR again.

@codecov-commenter
Copy link

codecov-commenter commented Jun 13, 2020

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##               dev     #6450   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          671       671           
=========================================
  Hits           671       671           

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 140ca83...ae7f4d8. Read the comment docs.

Copy link
Member

@GoodBoyDigital GoodBoyDigital left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@bigtimebuddy bigtimebuddy added the ✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t label Jun 16, 2020
@bigtimebuddy bigtimebuddy added this to the v5.3.0 milestone Jun 17, 2020
@bigtimebuddy bigtimebuddy merged commit cdf9494 into dev Jun 18, 2020
@bigtimebuddy bigtimebuddy deleted the dev-core-independence branch June 18, 2020 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✅ Ready To Merge Helpful when issues are in the queue waiting to get merged. This means the PR is completed and has t
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants