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

Skip flush in renderAdvanced if the container doesn't have an enabled… #7760

Merged
merged 6 commits into from
Sep 20, 2021

Conversation

dev7355608
Copy link
Collaborator

Description of change

I think we can skip the flushes in renderAdvanced if the container doesn't have an enabled filter or enabled mask.

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

@ShukantPal
Copy link
Member

renderAdvanced should only be called when a filter or mask is active on the display object. I don't think we should have extra checks for that.

@dev7355608
Copy link
Collaborator Author

renderAdvanced should only be called when a filter or mask is active on the display object. I don't think we should have extra checks for that.

But renderAdvanced is also called if the mask and filters are disabled:

if (this._mask || (this.filters && this.filters.length))
{
this.renderAdvanced(renderer);
}

We could also improve the check in render instead.

@ShukantPal
Copy link
Member

ShukantPal commented Aug 31, 2021 via email

@dev7355608
Copy link
Collaborator Author

Then the question is: do we move this block in render?

if (!this._enabledFilters)
{
this._enabledFilters = [];
}
this._enabledFilters.length = 0;
for (let i = 0; i < filters.length; i++)
{
if (filters[i].enabled)
{
this._enabledFilters.push(filters[i]);
}
}

This would be a breaking change probably.

@ShukantPal
Copy link
Member

Hmm, never mind. I see what you're doing here. If there are no filters enabled and no masking is done, we could do an early return right?

@dev7355608
Copy link
Collaborator Author

We could also move the flushes out of renderAdvanced and add them to FilterSystem/MaskSystem's push/pop, which should make more sense as well. Container is currently flushing for the filter/mask system, but these systems can do it themselves.

@dev7355608
Copy link
Collaborator Author

Hmm, never mind. I see what you're doing here. If there are no filters enabled and no masking is done, we could do an early return right?

If all filters and the mask are disabled, the container could be rendered non-advanced and should not trigger flushes.

Copy link
Member

@ShukantPal ShukantPal left a comment

Choose a reason for hiding this comment

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

Nice one. Any reason for being in draft?

@dev7355608
Copy link
Collaborator Author

Nice one. Any reason for being in draft?

I wasn't sure if this is the best solution. I'd prefer to do the flushing in the filter/mask system instead of in Container.

@dev7355608
Copy link
Collaborator Author

There were two mistakes in my code:

  1. I accidently moved the first flush after the filters are pushed.
  2. We need to flush if autoDetect is enabled even if the mask type is NONE.

This check is very long and ugly. We should really let the mask/filter system do the flushing.

@bigtimebuddy bigtimebuddy added this to the v6.2.0 milestone Sep 5, 2021
@dev7355608 dev7355608 marked this pull request as ready for review September 18, 2021 11:52
@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 20, 2021
@bigtimebuddy bigtimebuddy merged commit 0cf76fd into pixijs:dev Sep 20, 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

4 participants