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

Build refactoring #95

Merged
merged 8 commits into from
Aug 20, 2020
Merged

Build refactoring #95

merged 8 commits into from
Aug 20, 2020

Conversation

ShukantPal
Copy link
Contributor

@ShukantPal ShukantPal commented Aug 14, 2020

Synopsis 📃

This change will modernize pixi-tilemap's codebase to be up-to-date with the latest PixiJS standards 😃! I've done the following changes:

  • types.d.ts: This declares @pixi/<packages> modules and whatever classes pixi-tilemaps takes from those packages. This is necessary b/c only pixi.js has typings, but we import classes from @pixi/* packages.

  • import/export: The ES6 import/export syntax makes things modular & allows us to build to UMD & ESM bundles.

  • api-extractor: Automated declaration file generation!

  • rollup: This allows us to generate the proper CJS, ESM, UMD, UMD minified bundles.

  • namespace pixi_tilemap: Removed that namespace from each file. index.ts wraps all the exported classes at one place instead.

@ShukantPal
Copy link
Contributor Author

@ivanpopelyshev You should consider removing one of the package-lock.json or yarn.lock shrink-wrap files (or migrate to pnpm - I'll help you there).

@ShukantPal ShukantPal mentioned this pull request Aug 14, 2020
@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

the umd build doesnt seem to work and looking at the code I am not sure if its wrapped at all.
I get an error: Exception was: ReferenceError: _pixi_display is not defined

You can test it at this branch with gdevelop if you like. Just swap mine out for yours.
4ian/GDevelop#1901

it comes up as undefined when gdevelop loads it. Mine gets a bit further - its not undefined, but fails further down.

A successfully loading example of a umd pixi library in gdevelop is pixi-multistyle-text, which is used by the bbcode extension

@ShukantPal
Copy link
Contributor Author

@blurymind The UMD build is wrapped:

https://github.com/SukantPal/pixi-tilemap/blob/74e19e538982ec0a549242d9e22629ea24e2456e/dist/pixi-tilemap.umd.js#L12

I think I know what the problem is. The UMD build of @pixi/display, for example, is wrapped in _pixi_display. But the UMD build of pixi.js is wrapped in PIXI & doesn't expose _pixi_display.

@ShukantPal
Copy link
Contributor Author

@blurymind Can you try again?

@blurymind
Copy link
Contributor

yes sure, thanks for helping me :)

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

It gets a bit further now. The library is imported by gdevelop, but the moment I try to use it I get

Uncaught TypeError: PIXI.tilemap.CompositeRectTileLayer is not a constructor
    at new RenderedTileMapInstance (D:\DEV\GD\GDevelop\newIDE\app\resources\GDJS\Runtime\Extensions\TileMap\JsExtension.js:364)
    at Object.createNewInstanceRenderer (ObjectsRenderingService.js:62)
    at LayerRenderer.getRendererOfInstance (LayerRenderer.js:118)
    at InitialInstanceJSFunctor.LayerRenderer.instancesRenderer.invoke (LayerRenderer.js:51)
    at 8152 (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at _emscripten_asm_const_iii (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at Array._B (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at Array.ib (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at hF (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at As (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at InitialInstancesContainer.IterateOverInstancesWithZOrdering.InitialInstancesContainer.IterateOverInstancesWithZOrdering [as iterateOverInstancesWithZOrdering] (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at LayerRenderer.render (LayerRenderer.js:204)
    at InstancesRenderer.render (index.js:117)
    at InstancesEditor._renderScene (index.js:728)

@ShukantPal
Copy link
Contributor Author

@blurymind I forgot to assign PIXI.tilemap to _pixi_tilemap. One sec.

@ShukantPal
Copy link
Contributor Author

Okay, try again :)

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

This change seems to have regressed it. I get this now when trying to load the module
ReferenceError: _pixi_tilemap is not defined
If you look at the documentation of how to use the library, we expect to find it at PIXI.tilemap
Is that something that needs to change?

@ShukantPal
Copy link
Contributor Author

Ugh. I thought the name was _pixi_tilemap, but it should've been pixi_tilemap. I pushed a fix for this. Sorry for these silly mistakes.

@blurymind
Copy link
Contributor

will this address the previous issue which is
PIXI.tilemap.CompositeRectTileLayer is not a constructor

I need to debug this a bit more

@ShukantPal
Copy link
Contributor Author

https://github.com/SukantPal/pixi-tilemap/blob/695b63aab09152017d39df508f96438bb6e20327/dist/pixi-tilemap.umd.js#L953

It should be there. Can you console.log(PIXI.tilemap)?

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

Ah interesting, now its loading fine, but I am getting a different error when trying to use it

Container.ts:428 Uncaught TypeError: Cannot read property 'transform' of null
    at t.Container.updateTransform (Container.ts:428)
    at t.Container.getLocalBounds (Container.ts:506)
    at t.get (Container.ts:669)
    at RenderedTileMapInstance.getDefaultWidth (D:\DEV\GD\GDevelop\newIDE\app\resources\GDJS\Runtime\Extensions\TileMap\JsExtension.js:494)
    at LayerRenderer.getInstanceWidth (LayerRenderer.js:92)
    at LayerRenderer._isInstanceVisible (LayerRenderer.js:178)
    at InitialInstanceJSFunctor.LayerRenderer.instancesRenderer.invoke (LayerRenderer.js:59)
    at 8152 (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at _emscripten_asm_const_iii (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at Array._B (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at Array.ib (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at hF (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at As (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:25)
    at InitialInstancesContainer.IterateOverInstancesWithZOrdering.InitialInstancesContainer.IterateOverInstancesWithZOrdering [as iterateOverInstancesWithZOrdering] (libGD.js?cache-buster=5.0.0-beta98-e240a7c80d1abab373f93f826c8ca5fa39b63774:51)
    at LayerRenderer.render (LayerRenderer.js:204)
    at InstancesRenderer.render (index.js:117)
    at InstancesEditor._renderScene (index.js:728)

@blurymind
Copy link
Contributor

I confirm i can log PIXI.tilemap, but some of the items in it come up with type errors

Capture

is this perhaps due to strict mode?

@ivanpopelyshev
Copy link
Collaborator

That's function-related stuff, its supposed to be like that

@ShukantPal
Copy link
Contributor Author

@blurymind Can you give me a test script using pixi-tilemap so I can debug this today evening?

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

@blurymind Can you give me a test script using pixi-tilemap so I can debug this today evening?

All I have at the moment is the PR branch getting it to work in gdevelop with an example json fileto use
4ian/GDevelop#1901

This used to work with a normal module, but after gdevelop was ported to pixi5 we are now required to use umd modules.

My pr adds a new tilemap game object which should work in the IDE, but doesnt yet work when you playtest the game.
It is also possible there is an error somewhere in my extension, so I am looking into that as a potential issue too

I wonder if we can test this on something simpler - like a minimal example project

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

I believe the problem is still in pixi-tilemap because the error I getwhen trying to use it is

Container.ts:428 Uncaught TypeError: Cannot read property 'transform' of null
    at RectTileLayer.Container.updateTransform

Btw I was getting the exact same problem on my port to a umd module, which is the other pr here

@ShukantPal
Copy link
Contributor Author

@blurymind I would open a separate issue for that - with a minimal reproduction. @ivanpopelyshev would be the guy looking at that 😅

@blurymind
Copy link
Contributor

Do we have any proof that pixi-tilemap works as a umd module in anything? :)

@ShukantPal
Copy link
Contributor Author

Since the codebase is cleaner now, I might be able to help you too there.

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 14, 2020

Up until now, there was no UMD module - so no. The error you're talking about is a runtime error - not something related to the UMD build (as far as I can see).

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

I have a feeling that this is something that comes from its umd nature or pixi5, but its hard to tell.
Btw you might be able to get a better testing ground by using it in this example project
https://github.com/Alan01252/pixi-tilemap-tutorial

granted you need to swap the pixi library there with the new umd one + upgrade pixi4 there to 5

@blurymind
Copy link
Contributor

I made this as a testing ground
https://github.com/blurymind/pixi-tilemap-tutorial/tree/pixi-umd
feel free to fork and play. The module loads, but fails to work

It is also possible that some of the code still needs changing to work with pixi5

@blurymind
Copy link
Contributor

blurymind commented Aug 14, 2020

I updated my demo project now to better test against pixi-tilemap as it is in dist on master for pixi 5.

The compiled version of the umd module does not work, the vanila version works fine.
You can comment one out and try with the other in the index.html to see.

To test on localhost you can use
npx http-server .

I am either not using your umd right, or it definitely does not work
https://github.com/blurymind/pixi-tilemap-tutorial/tree/pixi-umd

Uncaught TypeError: Cannot read property 'generateSampleSrc' of undefined
    at generateFragmentSrc (pixi-tilemap.umd.js:701)
    at new RectTileShader (pixi-tilemap.umd.js:745)
    at new TileRenderer (pixi-tilemap.umd.js:780)
    at r.initPlugins (pixi.min.js:1)
    at new r (pixi.min.js:1)
    at Function.create (pixi.min.js:1)
    at Ur (pixi.min.js:1)
    at new t (pixi.min.js:1)
    at (index):20

@blurymind
Copy link
Contributor

blurymind commented Aug 19, 2020

Have you managed to do some testing with the project I provided? Is there anything else I can do to help this PR? It's something we need to get pixi-tilemap working again in gdevelop and blocks my PR there atm

I'm sorry I dont have quite as much experience with rollup. Perhaps we can try manually editing the resulting file to get it to work somehow?

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 19, 2020 via email

@ivanpopelyshev
Copy link
Collaborator

I can accept other PR for now, if you are not sure about this one.

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 19, 2020 via email

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 19, 2020 via email

@ShukantPal
Copy link
Contributor Author

@blurymind I made a fix, can you recheck?

@blurymind
Copy link
Contributor

Thank you, I will give this a try after work :)

@blurymind
Copy link
Contributor

@SukantPal that fixes it! The UMD build works in the example minimal project now!

@ivanpopelyshev ivanpopelyshev merged commit 4c9dbdb into pixijs-userland:master Aug 20, 2020
@ivanpopelyshev
Copy link
Collaborator

ok, gonna re-publish it soon

@blurymind
Copy link
Contributor

ah wait, there is something wrong with this

@blurymind
Copy link
Contributor

sorry this might be a gdevelop related issue. It still fails in gdevelop unfortunately. I am trying to find out if its due to the module or gdevelop itself

@ivanpopelyshev
Copy link
Collaborator

Hell, whats wrong?

@ShukantPal
Copy link
Contributor Author

😆

@blurymind
Copy link
Contributor

blurymind commented Aug 20, 2020 via email

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 20, 2020 via email

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 20, 2020 via email

@blurymind
Copy link
Contributor

@SukantPal opened a new ticket here
#97

@blurymind
Copy link
Contributor

Florian left a very helpful comment at the issue I linked. Perhaps it can help you with the followup PR?
I can help with testing the fix in electron

@ShukantPal
Copy link
Contributor Author

ShukantPal commented Aug 23, 2020 via email

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