DataUtils: Clamp param of toHalfFloat().

[Mugen87]

Related issue: Fixed #22435.

Description

see #22435 (comment)

This PR ensures that DataUtils.toHalfFloat() clamps the parameter to the maximum representable value in float16.

I've manually patched RGBELoader with this fix in the following live example to demonstrate how it resolves the rendering glitch: https://glitch.com/edit/#!/plastic-copper-hyacinth?path=index.html%3A520%3A22

@bhouston Does this change look good to you?

[WestLangley]

Hmm..., I'm not sure that is where the test should be. That is where a warning should be.

The app should not try to convert a number to half-float that exceeds the maximum-supported value.

As implemented, this will cause an RGB hue shift, since each channel is processed separately.

[WestLangley]

Before, the method was producing the wrong answer for large values.

Now it is still producing the wrong answer: converting back to float will not yield the original value.

I think a waring is more appropriate -- and each app should handle large values in a manner appropriate for that app.

[bhouston]

I think that we should auto clamp when using RGBELoader with data type half. RGBE is natively a Float32 format and converting to Half is going to always be possibly problematic - just sometimes RGBE images may not be full dynamic range. Thus I do not think we have to warn here, it is a known problem with RGBE to Half.

Maybe a flag on the toHalfFloat that includes a clamp option is best? So you can enable or not enable it depending on use case? So you do not always do it by default.

[Mugen87]

Maybe a flag on the toHalfFloat that includes a clamp option is best? So you can enable or not enable it depending on use case? So you do not always do it by default.

We could introduce a second parameter clamp for this. And then just do:

if ( clamp === true ) val = Math.min( val, 65504 );

[WestLangley]

I think that we should auto clamp when using RGBELoader with data type half

Of course, RGBELoader must be fixed. It could clamp each channel separately or clamp and retain hue.

Maybe a flag on the toHalfFloat that includes a clamp option is best?

I disagree. Just clamp without a flag. If you want to clamp silently without warning, that is OK.

Why would someone want to set clamp to false and return garbage on purpose?

[bhouston]

Of course, RGBELoader must be fixed. It could clamp each channel separately or clamp and retain hue.

Ah, I misunderstood. You want it to figure out if any of RGB is above the threshold and then scale down the value uniformly to keep the hue.

[WestLangley]

@bhouston Yes. Fixing the calling apps is a separate issue.

It is up to you and @Mugen87 if a warning should be made when clamping in this PR.

[bhouston]

To be honest, you would want to scale down all colours in the HDR map together. basically you figure out the max value in the RGBE image and then scale it down so that the max fits into the half range -- basically a linear tone mapping operator and maybe also pass back the scaling factor used so you can put that into the intensity value of the envMap. To only scale down the biggest ones would really wreck the local continuity between pixel intensities and cause issues.

I think that clamping is fine and probably better than individual pixel scaling - less artifacts. Best case is the overall scaling of the whole image via linear tone mapping into the via half range.

[WestLangley]

To be honest, you would want to scale down all colours in the HDR map together.

In theory, yes. But we are talking about only the brightest of the brightest pixels, and for that, I think scaling down just those pixels is fine. Remember, it will be tone mapped, and ACESFilmic, for example, will desaturate the brightest colors anyway.

//

I still think the problem is not with toHalfFloat() -- it is with the calling app.

This PR is not applying the fix in the correct place.

[Mugen87]

The problem is that other callers of toHalfFloat() might require the same fix and eventually we are going to put the same code over and over again at different places. I think it's better to keep the PR as it is and enhance to toHalfFloat().

If necessary, we can mention the clamping in the documentation. Something like:

Values bigger than the maximum representable value of half float (65504) are clamped.

[WestLangley]

The problem is that other callers of toHalfFloat() might require the same fix and eventually

1. There are many methods in the library into which invalid values can be passed. We do not change the users data silently.

2. We do not validate the users's data. (See the Color class.)

3. Before, the method was returning the incorrectly-encoded value. Now, it is still returning the incorrectly-encoded value. This PR is not a "fix".

4. This approach will silently cause hue shifts in the RGB use case.

5. This approach is absolutely incorrect for non color data.

Based on those reasons, I'd post a waring. At least for a while so we can see where the errors are.

I do not think clamping is appropriate here. Clamping would be appropriate in RGBLoader for example. That is where the problem is.

[mrdoob]

Thanks!