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

Upgrade Pixi.js to 6.1.2 #3026

Merged
merged 1 commit into from
Sep 13, 2021
Merged

Upgrade Pixi.js to 6.1.2 #3026

merged 1 commit into from
Sep 13, 2021

Conversation

ClementPasteau
Copy link
Collaborator

The first purpose was to fix the issue #2644.
I tested the issue example and it is indeed fixed!

A lot of other fixes (and features) have been bundled in this release too: https://github.com/pixijs/pixijs/releases/tag/v6.1.0

What do you reckon is the best way to test such an upgrade @4ian ? Manual tests on multiple games ?

@Bouh
Copy link
Collaborator

Bouh commented Sep 10, 2021

I would say

  • Read the previous PixiJS patch notes for checking any deprecated methods or updated.
  • Add all objects one by one by adding them to the scene or check examples with these objects.
  • Start a preview with the different objects.
  • Check all effects. See here
    Checking the effects is important PIXI.Filter is based on PixiJS core.

@Bouh
Copy link
Collaborator

Bouh commented Sep 10, 2021

Visual unit test are also a good thing for the future :)
https://github.com/zaqqaz/visual-unit-tests

@4ian
Copy link
Owner

4ian commented Sep 12, 2021

I would say

  • Read the previous PixiJS patch notes for checking any deprecated methods or updated.
  • Add all objects one by one by adding them to the scene or check examples with these objects.
  • Start a preview with the different objects.
  • Check all effects. See here
    Checking the effects is important PIXI.Filter is based on PixiJS core.

Seems like a good list. The idea is to check manually all objects and then a bunch of more or less random example to hope to catch any regression according to the changelog. Seems like this upgrade is fairly low risk overall!

@ClementPasteau
Copy link
Collaborator Author

Nice! I'll try to do that at the end of the day

@ClementPasteau
Copy link
Collaborator Author

did it a bit earlier :)

The only thing that's different is the Blur Layer effect.
Previously, it seemed to crop the image, based on the number of render passes being done. (1 was cutting the image in half, 2 was hiding the image completely ??)
I assume this is a bug fixed rather than a bug introduced, am I wrong? @Bouh

ezgif com-gif-maker

@Bouh
Copy link
Collaborator

Bouh commented Sep 13, 2021

I assume this is a bug fixed rather than a bug introduced, am I wrong? @Bouh

Correct it's like an unexpected fix ^^

Let me compile GDevelop with these changes and check. We are never too much to check this big API change.

@4ian
Copy link
Owner

4ian commented Sep 13, 2021

Looks solid, and maybe we got a bugfix for free! @Bouh let us know if you find anything else, otherwise I think we can go ahead!

@Bouh
Copy link
Collaborator

Bouh commented Sep 13, 2021

Sounds good to me for the runtime and IDE!

  • One thing on the developer side, the .map file shouldn't be updated too?

    //# sourceMappingURL=pixi-legacy.min.js.map

    Users often interpret this error as the cause of the bug in their games.
    Basically, the .map files errors scare people and don't inspire confidence.

  • As the effects are also on objects I've tested some effects on objects and they work fine too.

@4ian
Copy link
Owner

4ian commented Sep 13, 2021

One thing on the developer side, the .map file shouldn't be updated too?

Good catch, should be otherwise we'll have surprises when trying to debug :) (i.e: wrong lines everywhere I guess)

@ClementPasteau
Copy link
Collaborator Author

interestingly the .map was never included 🤔

I'll add it so we're in a better position if a problem occurs in the future

@ClementPasteau
Copy link
Collaborator Author

I believe this will need more work than expected to add the .map for this file.
I couldn't get it to work and I can't find a similar example where a map is added directly from the code when it is not part of an extension.

As this is not causing any error in the console (it's always been like that) I'm suggesting to merge like this and we can see later if we really need the map.

@4ian
Copy link
Owner

4ian commented Sep 13, 2021

Yep let's go ahead as things are not worse (they are even strictly better if there are no regressions :)) than before!

@ClementPasteau ClementPasteau merged commit fc23517 into master Sep 13, 2021
@ClementPasteau ClementPasteau deleted the upgrade-pixi branch September 13, 2021 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants