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

RGBMLoader: Fix alerts in UPNG.js. #22578

Merged
merged 1 commit into from
Sep 23, 2021
Merged

RGBMLoader: Fix alerts in UPNG.js. #22578

merged 1 commit into from
Sep 23, 2021

Conversation

Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Sep 23, 2021

Related issue: -

Description

This PR fixes the lgtm.com alerts in RGBMLoader. Since these issues came from the UPNG.js code, I've reported them at the original repository. The maintainer quickly fixed them (one of them was actually a minor bug). We can now apply the code changes to RGBMLoader.

photopea/UPNG.js#71

@mrdoob mrdoob added this to the r133 milestone Sep 23, 2021
@mrdoob mrdoob merged commit 8cb903d into mrdoob:dev Sep 23, 2021
@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2021

Thanks!

@mrdoob
Copy link
Owner

mrdoob commented Sep 23, 2021

Although we should start considering removing it though. I think Safari 15 no longer needs it?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 24, 2021

It seems RGBM is purposely used by developers in favor of other HDR formats (independently on iOS). I'm not sure we should remove it and force users to a migration task. IMO supporting RGBM in the library is not really a huge deal.

@mrdoob
Copy link
Owner

mrdoob commented Sep 24, 2021

I meant removing UPNG.js

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Sep 24, 2021

Oh, okay. Interesting point! I did not have the chance to test it but does iOS 15 still premulitplying alpha on PNG loading?

@mrdoob
Copy link
Owner

mrdoob commented Sep 24, 2021

Hmm, I remember testing this a few months ago with Safari Technology Preview and seemed like they finally solved it. I have not tried in the release yet.

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.

2 participants