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

Fix the DisplayObject#cacheAsBitmap issue #5784 #5841

Merged
merged 2 commits into from Jun 26, 2019

Conversation

mofnire
Copy link
Contributor

@mofnire mofnire commented Jun 26, 2019

Description of change

This PR is to fix #5784. There are two problems in DisplayObject#_initCachedDisplayObject.

  1. The first one is renderer._activeRenderTarget does not hold a current rendering target.

https://github.com/pixijs/pixi.js/blob/4d71fa06ea787d81651fc3e624610ccfc3c3f457/packages/mixin-cache-as-bitmap/src/index.js#L185-L189

I replaced it with const cachedRenderTarget = renderer.renderTexture.current;.

  1. The second one is renderer.projection.transform is not restored after the line 213.
    I changed the method so that the projection transform is restored.

https://github.com/pixijs/pixi.js/blob/4d71fa06ea787d81651fc3e624610ccfc3c3f457/packages/mixin-cache-as-bitmap/src/index.js#L210-L216

Working example is https://www.pixiplayground.com/#/edit/hk1gjmUM84Nl72GncOii~.

*Please note I could not find the way of writing tests of features like this :( *

Pre-Merge Checklist
  • Tests and/or benchmarks are included
  • Lint process passed (npm run lint)
  • Tests passed (npm run test)

@codecov-io
Copy link

codecov-io commented Jun 26, 2019

Codecov Report

Merging #5841 into dev will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev    #5841      +/-   ##
==========================================
- Coverage   70.55%   70.54%   -0.02%     
==========================================
  Files         205      205              
  Lines       10400    10402       +2     
==========================================
  Hits         7338     7338              
- Misses       3062     3064       +2
Impacted Files Coverage Δ
packages/mixin-cache-as-bitmap/src/index.js 53.04% <0%> (-0.66%) ⬇️

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 00fd9d9...1804bf9. 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.

Thanks for the demo to help verify. You’re on a roll @mofnire!

@bigtimebuddy bigtimebuddy added this to the v5.1.0 milestone Jun 26, 2019
@ivanpopelyshev
Copy link
Collaborator

ivanpopelyshev commented Jun 26, 2019

OK, i know how to fix that, good catch! The idea is to store renderer.renderTexture.sourceFrame , not projection.transform.

That's how we generally bind renderTexture: renderer.renderTexture.bind(rt); That's how its fone in FilterSystem: renderer.renderTexture.bind(rt, sourceFrame);

Third param, destinationFrame, is never actually used in tree-related operations, I'll mark that parameter as wont be restored after temporary bind

I'll do separate PR and use your example to test it.

Copy link
Collaborator

@ivanpopelyshev ivanpopelyshev left a comment

Choose a reason for hiding this comment

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

Nope, you are completely right, we need exactly those lines to solve that issue. I dug up another one, but sourceFrame doesn't save it, so i'll form my PR separate from yours after we merge your fix.

Cursed playground: https://www.pixiplayground.com/#/edit/62HZx5glfSl7R1TJe_5aH

@bigtimebuddy bigtimebuddy merged commit 1555f5a into pixijs:dev Jun 26, 2019
@stevenalanstark
Copy link

Thank you very much for this, well done! :D

@mofnire mofnire deleted the dev-fix-cacheasbitmap branch June 27, 2019 00:41
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

Successfully merging this pull request may close these issues.

Cannot Create a texture from a Graphic if CacheAsBitmap is on
5 participants