Skip to content

Conversation

@vonvick
Copy link
Contributor

@vonvick vonvick commented Feb 6, 2025

What does this PR do?

This PR fixes the issue where the assets in locally packaged apps are unresolved and fail to load. Now the local assets are loaded successfully and served to the app.

How has this been tested?

This has been tested locally on various WebOS devices

@wouterlucas
Copy link
Contributor

Thanks! I guess this would not work with Image workers set to 0 right?

Because then it'll fall back to:
https://github.com/lightning-js/renderer/blob/main/src/core/textures/ImageTexture.ts#L223

@vonvick
Copy link
Contributor Author

vonvick commented Feb 6, 2025

I need to test that with that setting. I am guessing that it would fail because it would be using fetch which does not support the file protocol. Do you reckon we can use the XMLHttpRequest there instead of the fetch?

Thanks! I guess this would not work with Image workers set to 0 right?

Because then it'll fall back to: https://github.com/lightning-js/renderer/blob/main/src/core/textures/ImageTexture.ts#L223

@michielvandergeest
Copy link
Contributor

@vonvick yes we should use XHR there as well, to ensure we can load local files on the web worker as well. To my understanding there are no performance implications between XHR and fetch when loading simple images. For (large) JSON files, fetch supposedly is faster

Also we need to add license headers for the new helper file you created (src/core/text-rendering/font-face-types/utils.ts)

Other than that, this looks good to me!

@wouterlucas
Copy link
Contributor

Loads of test failing though, likely because something is tripping the MSDF Font parsing. Had blurry fonts before if the alpha channel was incorrectly set.

And there seems to be a mismatch now with loading some textures.

@vonvick
Copy link
Contributor Author

vonvick commented Feb 7, 2025

@wouterlucas it did fail with the ImageWorker set to 0. But I have fixed it and now it works with or without the image worker being used.

cc @michielvandergeest

Copy link
Contributor

@michielvandergeest michielvandergeest left a comment

Choose a reason for hiding this comment

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

LGTM! 🔥

@michielvandergeest michielvandergeest added this pull request to the merge queue Feb 10, 2025
Merged via the queue into lightning-js:main with commit a4a5a81 Feb 10, 2025
2 checks passed
@wouterlucas
Copy link
Contributor

we should really rename fetchJson to be something more generic then that, since there is no json parsing anymore and its being used for images as well 😄

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