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 to PixiJS v6 #2447

Merged
merged 8 commits into from
Apr 9, 2021
Merged

Upgrade to PixiJS v6 #2447

merged 8 commits into from
Apr 9, 2021

Conversation

4ian
Copy link
Owner

@4ian 4ian commented Mar 18, 2021

For now just a quick test to see how it works and to get a nightly build generated with it for the runtime.

  • Upgrade in the IDE (package.json)
  • Upgrade in the Runtime (file with the minified version)
  • Do adaptations:
  • Verify all filters work.
  • Update types

Will fix #1991.

Autogenerated testing versions (be careful, please backup your game):

@4ian 4ian marked this pull request as draft March 18, 2021 22:52
@Bouh
Copy link
Collaborator

Bouh commented Mar 18, 2021

Moved drawStar Method

Graphics drawStar method has been moved to @pixi/graphics-extras, if you're using that, you need to use this optional package (not included in pixi.js or pixi.js-legacy by default). This method was added before graphics-extras and belongs with those optional drawing functions.

Source
Source2

I need to test it and see for fix.

@@ -0,0 +1,406 @@
/*!
Copy link
Contributor

@arthuro555 arthuro555 Mar 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should use the minified version? We can download the sourcemaps to make debugging easier. (Same for pixi.js)

@Bouh
Copy link
Collaborator

Bouh commented Mar 20, 2021 via email

@arthuro555
Copy link
Contributor

I already did the main work in 629adcc, you just need to have the .map file sit next to the normal file and have the same name except for the .map

@4ian
Copy link
Owner Author

4ian commented Mar 20, 2021

Yep let's use the minified version + the .map next to it :)
Btw I put the non minified version for pixi.js just to quickly test and help if there are crashes, but we can do the same I think.

@Bouh
Copy link
Collaborator

Bouh commented Mar 20, 2021 via email

@Bouh
Copy link
Collaborator

Bouh commented Mar 24, 2021

Fix for texture on bitmapText is out in v6.0.1 🎉

@4ian
Copy link
Owner Author

4ian commented Mar 24, 2021

Great! 👍
I think the biggest "challenge" is pixi-tilemap then. The rest seems straightforward/not changed?

@Bouh
Copy link
Collaborator

Bouh commented Mar 25, 2021

Nothing shocked me in the recent patch notes of PixiJS.
Could be fair enough to check the filters, and run a bunch of examples to be sure.
I can take care of it tomorrow in the end of the day.

@4ian
Copy link
Owner Author

4ian commented Mar 25, 2021

That's good yes if you can take a look at the whole app. We'll also have to update Pixi.js inside the IDE, but if it works for the runtime then it will probably work without a single issue in the IDE, as we use much less features :)

@Bouh
Copy link
Collaborator

Bouh commented Mar 26, 2021

The project for test layer effects are in GDJS/tests/games/effects/all effects demo.json
Everything is ok!
comparator_effect

@4ian
Copy link
Owner Author

4ian commented Mar 26, 2021

Awesome! If we can get this "rolling" and merged quickly, we can then merge BitmapText soon after :)

@Bouh
Copy link
Collaborator

Bouh commented Mar 27, 2021

I've open up, run, and edited some examples with all types of objects, nothing to signal.
Expect one deprecation warning for tilmap.
image

For my test i have copied this branch and merged with master.

I let you check for update tilemap.

@4ian
Copy link
Owner Author

4ian commented Mar 27, 2021

Thank you I'll check how tilemap is working :) If there are no bugs we can also go ahead with Pixi.js v6 and upgrade to pixi-tilemap v3 later.

@blurymind
Copy link
Contributor

for tilemap, do we still need this hack?
76b904a

@4ian
Copy link
Owner Author

4ian commented Apr 4, 2021

for tilemap, do we still need this hack?

It's properly integrated now in pixi-tilemap it seems.

Speaking of tilemaps, I've tested the two examples and everything seems to be working fine with Pixi.js v6.0.1. So I think we don't need to update to pixi-tilemap v3 directly, we can do this in a separate task :)

I've tested a larger game that works well, filters seem all to work as tested by @Bouh, so seems we're good to go? :)
This will solve a performance issue on iOS so it's fairly important, and this will also unblock BitmapText! :)

@4ian 4ian marked this pull request as ready for review April 4, 2021 10:06
@Bouh
Copy link
Collaborator

Bouh commented Apr 4, 2021

Then sound good to me!

@4ian
Copy link
Owner Author

4ian commented Apr 5, 2021

We're actually missing one last point: the typings! When upgrading pixi.js in the GDJS/package.json, we have tons of typescript errors, because it seems that types are declared differently and so PIXI is not recognised :/
Painful!

@arthuro555
Copy link
Contributor

No offense intended but I don't think the last commit is a good solution. Casts to unknown, trying to force pixi types into a namespace by from what I understood patching the original PIXI file and importing pixi many times in the type definition... All of this add a lot of complexity to the code and makes us bypass type checking, which beats the purpose of typescript.

Then, I haven't really looked into it so if it is the only way, I guess there is no avoiding this.

@4ian
Copy link
Owner Author

4ian commented Apr 7, 2021

No worries!
Happy to wait if you want to give a try at a different solution. I've unfortunately not found anything else that would allow to use PIXI as an "ambient namespace", meaning that you can just write PIXI.xxx and it "just works". The only way was to put it in a module.

I'm also not satisfied at having to modify pixi.js to add this global variable - this is really TypeScript running in our way there. I could have added this at some other place but I need to do it immediately after PIXI is defined, so I thought it was the best place.

If anything this will help migrating toward modules in the long term rather than using a global PIXI object (i.e: in the future we can replace the import PIXI = GlobalPIXIModule.PIXI by import * as PIXI from "pixi.js" or equivalent). But this is not a priority at all and would need heavy refactoring of all the files to use import/export instead of namespace gdjs { (and extensions too).

Let me know if you think of something better.
The as unknown are somehow necessary (not sure why) but as PIXI.filter.xxx was already there before, so we're not making things worse in terms of typing.
In other words, we've not lost any type checking coverage with this.

@4ian
Copy link
Owner Author

4ian commented Apr 8, 2021

@arthuro555 Let me know if you have time to look at this. Would be great to go ahead with PixiJS v6 soon as it will allow BitmapText to get in, and also it has a performance fix for games on iOS :)

@arthuro555
Copy link
Contributor

I took a longer look than I should have, and haven't found a better way. The only other way I found was using this:

import * as PIXI from 'pixi.js';
export as namespace PIXI;
export = PIXI;

But I didn't manage to extend the namespace with pixi-filters types without crashing the typescript compiler. I guess that having a weird hack is better than losing type coverage on all filters.

@4ian
Copy link
Owner Author

4ian commented Apr 9, 2021

Nice finding though this export as namespace, I've been searching everywhere in the doc. Now I guess the filters are causing a crash 😅 I'll give another quick try, in the worse case we can always go back to this later.

@4ian
Copy link
Owner Author

4ian commented Apr 9, 2021

Seems like as soon as you try to use a namespace PIXI after using export as namespace PIXI, you're getting a crash.
I think we've lost enough time on this. Filters are a bit suboptimal in the sense that they declare a different namespace PIXI as GlobalPIXIModule, but apart from that everything is still typed and the import PIXI = GlobalPIXIModule.PIXI actually help in the future to identify all files using PIXI.
So I think we'll go ahead like this!

@4ian 4ian merged commit 54ef748 into master Apr 9, 2021
@4ian 4ian deleted the upgrade/pixi-v6 branch April 9, 2021 22:58
@Bouh Bouh mentioned this pull request Sep 10, 2021
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.

Issues with iOS 14 and desktop Chrome (current build)
4 participants