-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
fix: alpha mask filter broken with render group cache-as-texture #11577
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
base: dev
Are you sure you want to change the base?
Conversation
|
pixi.js-base • pixi.js-bunny-mark commit: |
|
hey @silenaker are you able to take a look at the failing visual tests? |
adbe59a to
d850e65
Compare
mayakwd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this fix appears to be a solution for a specific case involving one filter. Of course, the mask is a specific case to begin with, so there's nothing wrong with that.
However, it may be worth revisiting this pull request and considering all filters and bounds calculations for RenderGroup.
Currently, filters “break” in unexpected places when the user uses cacheAsTexture.
src/filters/FilterSystem.ts
Outdated
| this._calculateFilterBounds(filterData, renderer.renderTarget.rootViewPort, rootAntialias, rootResolution, 1); | ||
| let viewport = renderer.renderTarget.rootViewPort; | ||
|
|
||
| const renderGroup = instruction.container.renderGroup || instruction.container.parentRenderGroup; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instruction.container is not always available. This line will break execution and produce "TypeError".
FilterInstruction contains either container or renderables.
0862077 to
9690b53
Compare
This isn’t a fix targeting a specific filter. The
On the surface, this PR fixes an issue where alpha mask doesn’t work properly inside cache-as-texture render groups. But while addressing that, I encountered several deeper issues in core components like During the review, I noticed several inconsistencies in how it handles nested filters, resolution, and antialias settings. The logic there isn't entirely clear and contains some conflicting intentions. I attempted to address these issues too, but the changes were too extensive and out of scope for this PR. As a result, I’ve only included the minimal changes necessary to keep this PR focused and stable and I'll create new PRs to address these not directly related issues. |
Description of change
AlphaMaskPipecalculates the bounds of the mask using world coordinates instead of the local coordinates of the cache-as-texture render group.getFastGlobalBoundsshould calculate local bounds only for render groups; otherwise, it may cause unpredictable bugs.FilterSystemappliesclipToViewportwith the screen root viewport instead of the cache-as-texture render group's viewport, causing unexpected bugs.Pooldoes not account for duplicate object returns, leading to erratic bugs and unit test failures.TexturePool#getOptimalTexturemethod does not dispatch texture update event when the texture frame is updated. As a result, theTextureMatrixthat depends on it fails to timely update its state (TextureMatrix#mapCoord), leading to errors when calculatinguFilterMatrixuniform inMaskFilter#apply.StencilMask,AlphaMaskshould not haveaddBoundsandaddLocalBoundsmethods, as they lead to incorrect bounds calculation on masked objects. No changes were committed since users can implement a new AlphaMask class to fix this.Pre-Merge Checklist
npm run lint)npm run test)