Skip to content

Conversation

@Mc-Pain
Copy link
Contributor

@Mc-Pain Mc-Pain commented May 21, 2025

Faster approach lead to some unpleasant render artefacts, use slower but accurate approach.

This also makes water reflect rays from skylight, bringing realistic Earth colors.

@fluffyfreak
Copy link
Contributor

Nice work. Would you mind giving some explanation of the above tropopause work in here just so that I can understand it.

@Mc-Pain
Copy link
Contributor Author

Mc-Pain commented May 21, 2025

Would you mind giving some explanation of the above tropopause work in here just so that I can understand it.

Model uses two different formulas for calculating pressure/density
One below tropopause (where temperature decreases with height)
One above tropopause (where temperature is constant)

The one above tropopause is not implemented yet.

@impaktor
Copy link
Member

Any pictures?

What would "slow" vs "fast" mean here, in performance? Would this put a burden on older machines that can still run pioneer?

@Mc-Pain
Copy link
Contributor Author

Mc-Pain commented May 22, 2025

Any pictures?

#5617 (comment)

What would "slow" vs "fast" mean here, in performance? Would this put a burden on older machines that can still run pioneer?

slow - O(n^2) complexity: 8 samples for calculating density for light ray
fast - O(n) complexity: 1 sample approximating density for light ray but it looks wrong near terminator

@Mc-Pain
Copy link
Contributor Author

Mc-Pain commented Jul 24, 2025

This fixes #6052

@fluffyfreak
Copy link
Contributor

There seem to be some commits that don't belong in this change history now

@Mc-Pain
Copy link
Contributor Author

Mc-Pain commented Jul 28, 2025

There seem to be some commits that don't belong in this change history now

fixed

Copy link
Member

@sturnclaw sturnclaw left a comment

Choose a reason for hiding this comment

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

Overall, I'm pretty happy with the perceptual quality on display here, though it does come at a potentially staggering performance cost.

In my performance testing, the baseline frametime with #define FAST 1 and 16 samples in the outer loop is ~9ms. I believe this to be the same effect as currently in master:

Screenshot From 2025-09-09 17-45-52

I'm using that as the baseline for the following measurements with #define FAST 0. For measurements which say "no variable sample count", I've removed the if-block which computes numSamples based on the distance through the atmosphere.

numSamples=16, no variable sample count (+0.1ms):

Screenshot From 2025-09-09 17-36-33

numSamples=16, max 256 samples; this PR as currently written (+6ms):

Screenshot From 2025-09-09 17-40-06

numSamples=8, max 64 samples (+1ms):

Screenshot From 2025-09-09 17-36-58

Finally for an idea of the cost and appearance of the cheapest possible version of the effect, I performed a test with numSamples=8, no variable sample count (-0.5ms):

Screenshot From 2025-09-09 17-36-50

Given the extremely small perceptual difference between the n=16, max=256, and n=8, max=64 cases, I cannot justify the performance cost of an additional 5ms on a high-end GPU. I'd like to see this PR adopt a setting of numSamples=8, numSamples = min(..., 64) before merge.

Eventually I'd prefer being able to specialize those numbers via a quality setting, for example a low-quality preset could support n=8, max=32, and a high-quality preset could go up to n=16, max=128, but that is somewhat out-of-scope for this PR.


// Eclipse data
Eclipse eclipse;
vec2 tropoHeight; // height for (R, M) in km, pressure is 0.1 atm
Copy link
Member

Choose a reason for hiding this comment

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

tropoHeight does not appear to actually be used anywhere.


vec3 I = normalize(eyeposScaled - planetIntersect);
vec3 center = I * geosphereRadius;
frag_color = vec4(0.f);
Copy link
Member

Choose a reason for hiding this comment

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

This is an early write to frag_color that has no effect.

vec4 diff = vec4(0.0);

vec2 atmosDist = raySphereIntersect(geosphereCenter, eyenorm, geosphereAtmosTopRad);
vec2 planetDist = raySphereIntersect(geosphereCenter, eyenorm, geosphereInvRadius);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you're using the inverse of the planet's view-space radius here to compute the planetary intersection? In testing, this actually does nothing, as you're attempting to compute an intersection in planet-radii space with an incredibly small sphere (smaller than 1/1000th the visible size of the planet).


vec2 atmosDist = raySphereIntersect(geosphereCenter, eyenorm, geosphereAtmosTopRad);
vec2 planetDist = raySphereIntersect(geosphereCenter, eyenorm, geosphereInvRadius);
vec3 planetIntersect = geosphereCenter + (eyenorm * planetDist.x);
Copy link
Member

Choose a reason for hiding this comment

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

Under all cases, the value of planetIntersect is equal to the center of the geosphere or well behind the planet in the incredibly tiny area (no more than a fraction of a pixel) where the geosphere's center lies exactly on the camera ray.

Comment on lines +63 to +64
vec3 I = normalize(eyeposScaled - planetIntersect);
vec3 center = I * geosphereRadius;
Copy link
Member

Choose a reason for hiding this comment

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

As a consequence, I=V under all cases (and produces graphical artifacts when it is not). Thus, the entire ray-sphere intersect test can be dropped entirely, and center be redefined as V * geosphereRadius.

float maxSegment = atmosphereHeight / numSamples;

if (height(tCurrent * dir, center) > 0 && numSamples * maxSegment < (boundaries.y - boundaries.x)) {
numSamples = min(int(ceil((boundaries.y - boundaries.x) / maxSegment)), 256); // max 256 segments
Copy link
Member

Choose a reason for hiding this comment

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

256 segments is quite excessive. The perceptual difference is small, and the performance cost is quite stark.

In testing with a 2070 Super (already a very hi-spec card by Pioneer's standards), just adding this branch with a maximum of 16 samples is a baseline cost of 0.2ms/frame, and with a scene set on Earth, a 64 sample maximum represents an additional cost of 2ms/frame.

At 256 samples, the additional cost of 6ms per frame(!) on high-spec hardware is decidedly well out of our performance budget, especially for the incredibly tiny perceptual difference from a 64 sample maximum.

These tests were performed at 1920x1080, paused, with all other details set to low so as not to introduce any other competing factors.

Comment on lines +229 to +231
if (altitude > m_tropopause) {
return double(GetAverageTemp()) - lapseRate_L * m_tropopause;
}
Copy link
Member

Choose a reason for hiding this comment

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

Note that this appears to set a constant atmospheric density above tropopause regardless of altitude. Is this actually physically correct?

If this is intended, this if-block isn't needed, as you can simply use std::min(altitude, m_tropopause) to replace the altitude scalar in the following line.

}

double tropopause_temperature = surfaceTemperature_T0 - lapseRate_L * m_tropopause;
m_atmosRadius = log(0.001 / 0.1) * GAS_CONSTANT_R * tropopause_temperature / (-surfaceGravity_g * GetMolarMass(GetSuperType())) + m_tropopause;
Copy link
Member

Choose a reason for hiding this comment

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

I'd appreciate a comment here explaining the resulting value of m_atmosRadius. At what pressure or altitude is the radius set?

//water only for specular
if (vertexColor.b > 0.05 && vertexColor.r < 0.05) {
CalcPlanetSpec(specularReflection, uLight[i], L, tnorm, eyenorm, WATER_SHINE);
if (vertexColor.b > 0.05 && vertexColor.r < 0.05) {
Copy link
Member

Choose a reason for hiding this comment

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

This test causes noticeable "speckling" on regular Mars terrain (i.e. not water) when looking in the direction of the sun. In testing, the vertexColor.b > 0.05 test needs to be increased to at least 0.15 to eliminate errant pixels from being classified as water.

@Mc-Pain
Copy link
Contributor Author

Mc-Pain commented Sep 10, 2025

@sturnclaw you just tested the scattering effects for the most common case possible: direct daylight
Please try again with twilight light on ground or at some altitude as near as possible to light/shadow border

UPD there are much more corner cases like atmosphere on Venus

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.

4 participants