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

Allow multiple objects to have the same mask #7754

Merged
merged 6 commits into from
Sep 13, 2021

Conversation

dev7355608
Copy link
Collaborator

Description of change

Multiple objects having the same mask is possible, but the code doesn't seem to be written with that in mind. isMask can easily end up being false even if the object is still used as a mask. The current implementation can cause problems if the mask object is before the object that is masked in the scene graph: the mask is rendered normally for one frame after isMask is set to false and renderable to true when the mask is removed from another object; isMask remains false, but renderable is set to false again by the mask system. Here a demo where the the mask is added and removed from some other object every frame and so the mask is rendered normally in the background. https://www.pixiplayground.com/#/edit/jC73hmNsD59Hgyp6kseVL. If the mask is after the all objects that use the mask in the scene graph, this visual bug wouldn't occur; only isMask would be incorrectly false.

I fixed this problem by adding a reference counter that counts the number of times the object is used as a mask.

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

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.

Overall I don't have an issue with this, however, I think your tests should also cover destroying elements to make sure the mask refs are updated too.

packages/display/src/DisplayObject.ts Outdated Show resolved Hide resolved
@dev7355608
Copy link
Collaborator Author

Overall I don't have an issue with this, however, I think your tests should also cover destroying elements to make sure the mask refs are updated too.

Destroy slipped my mind. Fixed that.

@bigtimebuddy bigtimebuddy added this to the v6.2.0 milestone Sep 8, 2021
@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 Sep 8, 2021
@bigtimebuddy bigtimebuddy merged commit 172b195 into pixijs:dev Sep 13, 2021
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

3 participants