-
Notifications
You must be signed in to change notification settings - Fork 785
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
Upgrade to Pixi.js 5.3.0 #1824
Upgrade to Pixi.js 5.3.0 #1824
Conversation
* Huge thanks to @Quarkstar for working on this task (original PR #1628)
Here a list of thing to check before merge this huge pixi-v5 branch. Before all i wish propose to continue ours effort and forward to the maximum version of PixiJS, i mean pass from 5.2.4 to 5.3.0. Because 5.3.0:
Thing to check :
|
Yep, no reason to stay behind, I've pushed a commit to use 5.3.0 in the editor :)
Thanks! I've made a quick check on various examples and could not find any problems so far.
There are still a few differences, we should check if some filters need an update/fix (or if it was the previous version that was wrong :)) |
Don't show in changelog
So I upgraded to Pixi.js 5.3.0, but turns out that now only 3 filters are shown in your example (didn't see that before commiting!) :/ I wonder if some filters need an update. Replacing |
Hiding the "Tilt shift" layer "fixes" the issue, the other layer and filters are now visible again. Might be some additional setup or stuff to do for this filter? EDIT: actually all layers shown below the layer with the tilt shift are not visible. |
I think the TiltShift issue when upgrading to latest Pixi.js 5.3.0 is because it's erasing all the previously rendered layers, due to pixijs/pixijs#6295 (or related) and the fact that TiltShift is missing a parameter when calling apply (the "clear"/"clearMode" parameter): https://github.com/pixijs/pixi-filters/blob/0ad9a90c7f78ff3e278fd2372da288439a8b6748/filters/tilt-shift/src/TiltShiftFilter.js#L34-L35 I'll make a hotfix in the existing filter code while waiting to report this. |
Don't show in changelog
I've started to test and fix Graphics, it's not yet finish, i've started an big project for test all the stuff.
Can I push the content of my local branch v5 on this PR ? |
When i use a combo of TimeDelta for an shape and ClearBetweenFrames (set on No, see the project file below of this post), i get warnings and this cause a crash of the game. The .json of the profiler, you can see the spike at the end in the timeline, the spike is the first warning. (The parameter for the action ClearBetweenFrames is already fixed i'll push it later.) Here the project used, go in scene Graphics and the group in red in the eventsheet. A part this warnings and crash, the Shape Painter Object work like expected. |
If you're disabling clear between frames, everything that you display in a frame is persisted for the next one, so basically you're filling an array of graphics with new stuff 60 frame per seconds... at some point this is becoming too much to render (and/or to hold in memory) and the whole game crashes because your GPU can't keep up (probably). That was my fear with this option, to risk having crashs like this when you add graphics to draw without stopping. We should surely have an upper bound of something like 100 shapes for a single object, and maybe add a warning somewhere. |
I've seen a hack for it in the pixi's guide and found a similar issue. This hack is now implemented in v5.3.0 but the issue persist. What you think if we use a renderTexture for keep the previous image of the shape painter object and use clear, this way the graphicData stay low. |
I think that's a solution, but if someone wants to use the Shape Painter objects to create a lot of small objects (i.e: a lot of objects, but each with a few shapes at most), it might be counter productive to create a texture for each of them.
|
I have a small suggestion for pixi-v5: using by default pixi-v5 and offer an option to enable legacy mode to use pixi-v5-legacy (with canvas rendering), like pixi does. |
It sounds like BitmapFont/Text is one potential fix for some of the text issues, and since 5.3 is required for that, wanting to just check to see if there was any status on this? Anything I could do to help with testing? |
I'm not gonna be here between Wednesday(tomorrow) and Sunday/Monday. |
Ran through what tests I could. Hopefully this is more helpful than hurtful.
|
Thanks both again for checking all of this :) Going forward let's be very strict on this checklist, notably:
I'll edit your last message @Silver-Streak with my comments :) |
Tilemap implementation will happen as a separate pr. I have zero interest
in piling it over this already huge pr, so no point in giving it a checkbox
here
…On Wed, 15 Jul 2020, 12:28 Florian Rival, ***@***.***> wrote:
Thanks both again for checking all of this :)
Going forward let's be very strict on this checklist, notably:
- if it's not an existing feature (tilemap, bitmap object), don't list
it. I know that Pixi v5 will unlock these, but for now we must concentrate
on landing Pixi v5, and if we add noise in this list, it will slow down my
ability to merge this and so my ability to then help/implement these
objects :)
- if it's an existing issue, let's not list it. Same, I would love to
fix all existing issues, but for now we must just ensure that there is no
regression. If it's not a regression, let's not write it down, it will slow
down Pixi v5.
I'll edit your last message @Silver-Streak
<https://github.com/Silver-Streak> with my comments :)
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1824 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABRRWVOSK3EEZUFZ63RDGELR3WHGJANCNFSM4OBSM4QA>
.
|
…they are working Thanks @Bouh for making this project. Don't show in changelog
Alright I think we've very near merging this!
Not blocking:
|
* Fix thickness by the thickness on the object. * Remove useless moveTo() Don't show in changelog
Merged! Thanks @Quarkstar again, @Bouh, @Silver-Streak and everyone that helped on this. @HarsimranVirk you should now be able to base your branch on the I know there was a few things that I listed in my previous message that we should still look at, but I think they are not blocking. It's also a large and important upgrade so better have it done because this could also solve some existing glitches/bug/performance issues from Pixi 4. I hope that apart from that there are no major regression - I could not find anything wrong when testing various game/examples - but let me know if there is something that seems not to work anymore! |
First, thanks again to @Quarkstar for working on the original PR #1628 (the PR will be merged in master using a rebase to ensure the commit is properly attributed).
Let's discuss/work here on what is needed to make Pixi.js v5 work perfectly with GDevelop - notably filters.
@Bouh if you want feel free to make a list here of what you think need to be done before we consider this branch ready :)
@HarsimranVirk You can now surely start from this branch
pixi-v5
and cherry pick your commits on top of it :) If you open a PR, you can choose to usepixi-v5
as the "base" branch, instead ofmaster
, so that only your commits are shown in the PR.