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

RGBELoader: Clamp prior to converting to half float #22451

Merged
merged 1 commit into from
Aug 31, 2021

Conversation

WestLangley
Copy link
Collaborator

Fixes #22435.

In lieu of #22444. (It is the calling app's responsibility to decide how to clamp large values.)

In this PR, the clamping is per-channel independently. Clamping to prevent a hue shift is also reasonable if users complain. But ACES Filmic shifts hue anyway, so for now, I think this is OK.

Below: Linear tone mapping and exposure set to 0.01. You can see how bright the sun is compared to the rest of the sky.

Screen Shot 2021-08-29 at 10 31 18 AM

@WestLangley WestLangley modified the milestones: 134, r134, r133 Aug 29, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 30, 2021

@Mugen87 I should merge this one instead of #22444, right?

@WestLangley
Copy link
Collaborator Author

I think a warning in #22444 instead of a clamp would be a good idea. That way, we will not be obfuscating the problem.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 30, 2021

I personally favored #22444. Now, I just want to see this issue fixed. I don't have a strong opinion anymore whether we clamp in the loader or in toHalfFloat() . Merge whatever you think it's best.

@mrdoob
Copy link
Owner

mrdoob commented Aug 30, 2021

I think a warning in #22444 instead of a clamp would be a good idea. That way, we will not be obfuscating the problem.

That makes sense to me actually. A warning in toHalfFloat() would help any "users" of that function.

@mrdoob
Copy link
Owner

mrdoob commented Aug 30, 2021

@WestLangley What do you think the warning message should be?

@WestLangley
Copy link
Collaborator Author

Maybe

console.warn( 'THREE.DataUtils.toHalfFloat(): value exceeds 65504.' );

three.js does not typically validate data, however. I was thinking a warning would be useful in alerting us to other possible problems. But that was just a thought...

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 31, 2021

How about: b85fa19

@mrdoob
Copy link
Owner

mrdoob commented Aug 31, 2021

How about: b85fa19

That looks good to me 👍

@mrdoob mrdoob merged commit a430cd9 into mrdoob:dev Aug 31, 2021
@mrdoob
Copy link
Owner

mrdoob commented Aug 31, 2021

Thanks!

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 31, 2021

Since you have merged both PRs, the clamping is now implemented two times. One should be sufficient, no 😇 ?

@mrdoob
Copy link
Owner

mrdoob commented Aug 31, 2021

Hmm, up to you.

I agree with @WestLangley that toHalfFloat() should warn the developer.
Whether it also clamps it or not I don't mind, but I think the clamp makes more sense in RGBELoader.

@Mugen87
Copy link
Collaborator

Mugen87 commented Aug 31, 2021

Whatever! At least we have one more closed issue 🎉 .

@mrdoob
Copy link
Owner

mrdoob commented Aug 31, 2021

No more UFOs! 😁

@WestLangley WestLangley deleted the dev_rgbe_loader branch August 31, 2021 11:05
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.

RGBELoader: Black pixels in high value areas
3 participants