Change filter clear parameter to clear modes #6295
Conversation
ivanpopelyshev
requested review from
GoodBoyDigital,
bigtimebuddy and
ShukantPal
|
Yep :) I'll do experiments which devices have this problem. Right now its just a paranoia option: v4 cleared everything, v5 doesnt and we have a switch between those modes. |
|
How are you supporting backward compatibility? I’m sure there are some pixi-filters using clear Boolean? I’d suggest adding a typeof check to clear and throwing a deprecation warning. |
|
Easy. 2 is not used in pixi-filters |
|
@ivanpopelyshev we should not use truthy conditions with constants like CLEAR_MODES. This is how we got bit with the scale modes, because someone flipped the value. Please don’t mix types. If someone uses a boolean, we should make sure that’s properly deprecated. |
|
We will get rid of booleans in pixi-filters and only numbers will remain. I just don't want the plugin to break right after we merge this thing. We won't have problems like that in future with typescript. |
Codecov Report
@@ Coverage Diff @@
## dev #6295 +/- ##
==========================================
- Coverage 77.98% 77.96% -0.02%
==========================================
Files 186 186
Lines 10111 10116 +5
==========================================
+ Hits 7885 7887 +2
- Misses 2226 2229 +3
Continue to review full report at Codecov.
|
test for it
…o dev-filter-clear-mode
|
OK, I've added support for |
| this.renderer.renderTexture.bind(filterTexture, filterTexture ? filterTexture.filterFrame : null); | ||
| if (clearMode === CLEAR_MODES.CLEAR | ||
| || (clearMode === CLEAR_MODES.BLIT && this.forceClear) | ||
| || clearMode === true) |
There was a problem hiding this comment.
This is an incorrect/incomplete way to implement backward compatibility. The API shouldn't support both booleans and CLEAR_MODES. Please move this condition (clearMode === true) out of the if statement so that we can easily remove it later. For instance, add this before the if:
// TODO: remove in next major version
if (typeof clearMode === 'boolean') {
clearMode = clearMode ? CLEAR_MODES.CLEAR : CLEAR_MODES.BLEND;
// get deprecation function from utils
deprecation('5.2.1', 'Use CLEAR_MODES when using clear applyFilter option');
}
You know that I can't see everything. Btw, tests show deprecation in console - can we do something about it? Also, I don't know whether we will remove booleans in next version, because i want to stay v4-compliant. |
|
If you see deprecation in console it means we missed a spot for upgrading. Could be a test? Probably worth tracking down. |
|
Both tests are mine: one uses BeginTextureFIll with arguments instead of object, and one here, I'm testing bool param. |
bigtimebuddy
changed the title
bigtimebuddy
changed the title
Previous issue #6270 #6257 and possible future changes in FilterSystem enabled me to finally clarify the
clearvariable everywhere.BlurFilter test for this branch: https://www.pixiplayground.com/#/edit/0-VaT0fFvoiWju_9XeJO2
Modes:
NO/BLEND(false) - don't clear the output texture, it contains vital information that we need to blend uponYES/CLEAR(true) - texture contains junk, clear it.AUTO/BLIT(new mode!) - we know that texture partially or fully cleared and previous state should not be a problem.By default pixi-v5 uses
renderer.filter.forceClear=false, which corresponds to minimal number of WebGL commands.The trick is - on some mobile videos its better to clear just after bind , because it saves the time for copying content. In that case we need
renderer.filter.forceClear=true.Another case is when someone made a mistake, like it existed in BlurFilter, but we dont have time to debug all those custom filters. In that case we just clear whenever its possible.
As a joke, I thought it was possible to add one more "russian" mode,
YES_NO_MAYBE(да нет, наверное). If filter area covers all renderTexture, we might not clear it borders and just blit it.