-
Notifications
You must be signed in to change notification settings - Fork 26
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
Add spritesheet animation handling. Also fix several minor issues #275
Conversation
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.
This is extremely comprehensive, great work. I'll have to review this a bit more over time (and when I have some more time after my move), but this is seriously impressive.
Yes, please take all the time you need. I'm also on vacation starting next week and will not be near a pc for two weeks. I will also try and test this in some more real world and complex animations. Maybe I'll also ask vauxs if he has some crazy stuff lying around :) |
Also fix scaleOut being affected by previous scaleIn
cb1ea16
to
0b6491d
Compare
I also finally managed to convert some of the jb2a assets to spritesheets to highlight the advantages of supporting spritesheets. beautiful.mp4 |
Woah, nice. Is that with the pre-multiply fix enabled or disabled? It's called the pixi fix in Sequencer's settings, it fixes the black edges around transparent effects |
I still need to re-enable effect masking for this code. I'm on it :) |
You mention:
|
I don't think there is... There might be a way to do it in chrome, but much slower and reslies on a video element, so I don't even know if that would easily work in the current worker configuration. This would utilize the But you're right.. Most people using a locally, self hosted foundry (just the app + ip sharing) will not provide a secure context. I should definitely add some checks so we don't even create the worker in that case, but I don't think we can really provide a good fallback solution |
I've added feature detection to not even download the sprite sheet worker in case a non-secure context is detected or VideoDecoder specifically is not available. Since my last update I also added |
Won't the following lines still execute even after the ad93b00#diff-f152739305fc0d3a79d9fe49be29db33dba514a5e8d06abbf5549893edcb7655R109 |
Damn it yes they would. Will fix :) |
Turns out shapes weren't broken to begin with in this branch, I just failed to reload my foundry instance when testing (regarding my comment in issue #334) |
It's happening!! |
This also addresses issues #266, #268, #269, #270, #271, #274 and #276 and can quite dramatically increase performance in cases where no filters or masks are applied to the effect.
Previously, each effect would essentially need 4 draw calls. One to paint the sprite texture, one for the always present alpha filter, and two more for the always present filters on the container for the faded and desaturated effect when viewing effects intended only for other players.
By not creating theses filters at all if they are not needed (like already done for the masks), these draw call overhead can be elimated. This not only removes the 3 uneccessary draw calls for the filters, but allows the effects texture drawing to be batched, for (for most systems) up to 16 effects in one draw call.
I've added a stresstest example to show the difference, though only the first part will be playable in the current version of sequencer.
A word on json spritesheets. I've included one free, CC0 animation as a spritesheet, flipbook and VP8 video file in this PR (flipbook version is already available in the other examples PR) to easily compare the performance of these different versions. Spritesheets have the simple but in some cases quite substantial advante of not needing seprate video decode step, as all frames of the animation are encoded in one base texture.
Furthermore, all effects playing the same animation share the exact same underlying base texture, wich means that duplicates of this animation have practically no inpact on performance at all. In the stresstest example, all 600 fire animations can be drawn in one single draw call using the spritesheet, which makes spritesheets predestined to be used for persistent animations that are duplicated quite often, like effects on tokens or small fire effects that might be placed all over a map.
As for the general reasoning behind these somewhat extensive changes:
I've decided to separate the asset/texture loading and the detailed sprite handling to another class, called
SequencerSpriteManager
.The main reason was, that I could not get the spritesheet unloading to reliably work in all cases because sometimes the files would destroy the shared spritesheet texture that was still in use by other effects.
Instead of litering the destroy dependencies and SequencerFile internal asset handling code with different special casing for spritesheets I thought it best to handle asset loading and unloading in one place and one place only.
Another reason for the added class is that starting with PIXI v8 (which will be introduced in Foundry VTT 13), sprites can no longer have children. Currently, a sprite is sometimes used as container and sometimes as a sprite displaying a texture itself.
Again, changing this without also breaking some existing effects, especially for those using animateProperty or loopProperty turned out to be quite complex. A seprate class that extends a simple container but imitates a sprite with its properties seemed the best solution to me to handle this in a backwards compatible way. At least as backwards compatibile as I could make it.
Of course, this had some far sweeping consequences for the
CanvasEffect
class itself, but it also allowed me to take a stab at fixing some of the existing issues in regards to animation looping and some strangeness when using texts on top of effects and how it affects the sprites anchor.I have done my best to verify that everything still works as before. All the examples / test cases added were used to compare the current sequencer version with this PR to make sure no new defects are introduced.