playcanvas / engine

Powerful web graphics runtime built on WebGL, WebGPU, WebXR and glTF
https://playcanvas.com
MIT License
9.73k stars 1.36k forks source link

Ionic path causes issues with asset loading #3491

Open lukasharing opened 3 years ago

lukasharing commented 3 years ago

https://github.com/playcanvas/engine/blob/3071b54d832815d6a3012f5d97bf5cbaf6f7741e/src/net/http.js#L492

This should also accept ionic path:

if (xhr.responseURL && (xhr.responseURL.startsWith('file:///') || xhr.responseURL.startsWith('ionic:///'))) {
yaustar commented 3 years ago

Hmm, wondering if we should replace this with a check for a generic :/// style address so we can handle multiple wrappers and frameworks?

For the short term, I would suggest using a fork off the stable branch if you aren't already.

lukasharing commented 3 years ago

The path file:/// is incorrect in some devices, it should be file:// and ionic:// not ionic:///

yaustar commented 3 years ago

The path file:/// is incorrect in some devices, it should be file:// and ionic:// not ionic:///

Hmmm, that makes it problematic to be flexible 🤔 . Will need some thought

yaustar commented 3 years ago

Thinking more on this, you could monkey patch the engine to make it work for your specific project to use your own logic.

lukasharing commented 3 years ago

Yes! We are using now a customized version of the engine 2021. We had to solve some bugs because we were using the version 2020 and these errors were not happening

yaustar commented 3 years ago

What other bugs did you find? Are they ones that we need to fix or are the specific for your project?

lukasharing commented 3 years ago

What other bugs did you find? Are they ones that we need to fix or are the specific for your project?

https://github.com/playcanvas/engine/issues/3483

yaustar commented 3 years ago

Ah yes, that one we should get fixed/merged

lukasharing commented 3 years ago

Looking at phaser 3, they use the path with 2 backslashes rather than 3, but they do not accept ionic path neither

https://github.com/photonstorm/phaser/blob/5cf01e2d6a32249faa237b70a5e7e3bf417837bd/src/loader/File.js#L311

yaustar commented 3 years ago

Thinking more on this, you could monkey patch the engine to make it work for your specific project to use your own logic.

Just to follow up on this, this is an example of a patch that can be made to a specific project: https://playcanvas.com/project/831967/overview/patch-http

yaustar commented 3 years ago

Looking at phaser 3, they use the path with 2 backslashes rather than 3, but they do not accept ionic path neither

https://github.com/photonstorm/phaser/blob/5cf01e2d6a32249faa237b70a5e7e3bf417837bd/src/loader/File.js#L311

The issue I'm thinking about is that we can't add a list to every wrapper that exists 😅

Wondering if there was a better/more generic/customisable way of doing this

lukasharing commented 3 years ago

a: xhr.responseURL.startsWith('file:///') -> This should be a redundant case b: xhr.responseURL.startsWith('file://') || because if b is true, then (a || b) is also true and if a is true, b is always true. -> (a || b) <=> b

yaustar commented 3 years ago

a: xhr.responseURL.startsWith('file:///') -> This should be a redundant case b: xhr.responseURL.startsWith('file://') || because if b is true, then (a || b) is also true and if a is true, b is always true. -> (a || b) <=> b

Sure, it was more to show how the patching would work in a project without needing to fork the engine