ebruneton / precomputed_atmospheric_scattering

This project provides a new implementation of our EGSR 2008 paper "Precomputed Atmospheric Scattering".
BSD 3-Clause "New" or "Revised" License
922 stars 121 forks source link

Mismatch between definition of meter in definitions.glsl and kLengthUnit in reference/model_test.cc #31

Closed 10110111 closed 5 years ago

10110111 commented 5 years ago

I was trying to add a dimensionful quantity into functions.glsl (Earth-Moon distance to experiment with solar eclipse in subsolar point), and it appeared that there's a mismatch between the units in which the GPU model works (supplies the values into the shader) and the units of the CPU model.

The particular variables which appear to be out of sync are:

The result is that the shader code used by the GPU model works in kilometers, but m is defined as 1.0, so that e.g. using 3*m in the shader will give you three kilometers instead of three meters.

A trivial fix would be to redefine m to 1e-3, but this seems to be a fragile solution, since any change to kLengthUnit will bring this out of sync again.

ebruneton commented 5 years ago

This is on purpose because:

To solve this we use different units on CPU and GPU. You need to do the conversion between the two when setting GPU variables from CPU, or in the shader. See for instance https://github.com/ebruneton/precomputed_atmospheric_scattering/blob/2bc8e4124e99bea7026fdd0a3933dd5c35ce6b5c/atmosphere/demo/demo.cc#L342 or https://github.com/ebruneton/precomputed_atmospheric_scattering/blob/2bc8e4124e99bea7026fdd0a3933dd5c35ce6b5c/atmosphere/demo/demo.glsl#L61

10110111 commented 5 years ago

The problem is not in different units, but rather in mismatch between the units and the value of the m constant. It is equivalent to 1 km for the GPU code and to 1 m for the CPU code, although I'd expect m to stand for "meter".

This doesn't break your original code, but behaves inconsistently when one tries to add a dimensionful constant using m to denote meters.

ebruneton commented 5 years ago

For me "m" still means 1 meter in the GPU code. But there is no automatic, transparent unit conversion in this code. This means that you must manually convert values in meters to values in the GPU length unit. In practice, this means dividing by kLengthUnitInMeters.

For instance https://github.com/ebruneton/precomputed_atmospheric_scattering/blob/2bc8e4124e99bea7026fdd0a3933dd5c35ce6b5c/atmosphere/demo/demo.glsl#L61 sets the sphere center at 1 km, and it could have been written with an explicit "m":

const vec3 kSphereCenter = vec3(0.0, 0.0, 1000.0 * m) / kLengthUnitInMeters;