Merge copy texture fix #1505
Merge copy texture fix #1505
Conversation
Polyfills/Canvas/Source/Canvas.cpp
Outdated
| } | ||
|
|
||
| return needClear; |
There was a problem hiding this comment.
Its expected that the Canvas is cleared when setheight/setwidth is set (even if they don't change)
Has this behavior has been preserved?
There was a problem hiding this comment.
Hmm, good question, it seems like the previous call to Clear is gone and needClear is now and unused local variable. If we can wait until tomorrow, let's ask @CedricGuillemet.
There was a problem hiding this comment.
Did this result in any regression? If not, I would say lets just merge this PR since it is already in master, and we can do a follow up PR in master after Cedric is back if needed and bring it into this branch. If it is causing a regression though, then let's wait a day for Cedric.
There was a problem hiding this comment.
Looks like there is a regression (in that it doesn't clear anymore). But should be solvable with a simple flush on setheight/width. Image free crashes seem to be resolved -- so I think we can just merge this now, and I can raise a PR after to fix canvas clear behaviour
There was a problem hiding this comment.
I tried to simplify the code too much and it caused the regression. Best seems to revert that part on this PR. Can you do the change and test it again @Pheo ? Sorry for the inconveniences.
There was a problem hiding this comment.
raised PR @ ryantrem#6
Confirmed working! Framebuffer correctly clears on canvas setHeight/setWidth.
There was a problem hiding this comment.
@CedricGuillemet can you make that same fix in master as well?
This brings in the changes from master to fix timing/sequencing issues related to copyTexture and deleteTexture.
This included changes on the JS side, so it brings with it new versions of the BJS packages.