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

Limb darkening #2047

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Limb darkening #2047

wants to merge 3 commits into from

Conversation

375gnu
Copy link
Member

@375gnu 375gnu commented Dec 30, 2023

published for testing and getting response.

image

With atmospheres it looks… weird.

@@ -2905,14 +2910,13 @@ ShaderManager::buildAtmosphereFragmentShader(const ShaderProperties& props)

// Sum the contributions from each light source
source += "vec3 color = vec3(0.0);\n";
source += "vec3 V = normalize(eyeDir);\n";
Copy link
Member Author

Choose a reason for hiding this comment

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

eyeDir is already normalized since we implemented per-pixel shading.

@@ -21,12 +21,14 @@ class Texture;

struct RenderInfo
{
Color color{ 1.0f, 1.0f, 1.0f };
Copy link
Member Author

Choose a reason for hiding this comment

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

reodering is to decrease the struct size

@@ -460,12 +460,15 @@ class Renderer
Surface* surface{ nullptr };
const Atmosphere* atmosphere{ nullptr };
RingSystem* rings{ nullptr };
LightingState::EclipseShadowVector* eclipseShadows{ nullptr };

Eigen::Quaternionf orientation{ Eigen::Quaternionf::Identity() };
Copy link
Member Author

Choose a reason for hiding this comment

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

reodering is to decrease the struct size

@Askaniy
Copy link
Member

Askaniy commented Dec 30, 2023

Is it just me, or is there a sprite (or something similar) in the center of the star that increases the brightness and causes some colors to overexpose?

@Askaniy
Copy link
Member

Askaniy commented Dec 30, 2023

I also think that atmospheres, as a workaround for the glow effect, should be removed

@375gnu
Copy link
Member Author

375gnu commented Dec 30, 2023

Is it just me, or is there a sprite (or something similar) in the center of the star that increases the brightness and causes some colors to overexpose?

Yes, there is a big quad (sprite) if front of the Sun.

@375gnu
Copy link
Member Author

375gnu commented Dec 30, 2023

That's how it looks with stars as scaled disks (in this mode the quad size is much smaller):
image

Copy link

sonarcloud bot commented Dec 30, 2023

@375gnu
Copy link
Member Author

375gnu commented Dec 31, 2023

btw, in old HDR code atmosphere is disabled:

#ifndef USE_HDR
        Atmosphere atmosphere;

        // Use atmosphere effect to give stars a fuzzy fringe
        if (star.hasCorona() && rp.geometry == InvalidResource)
        {
            Color atmColor(color.red() * 0.5f, color.green() * 0.5f, color.blue() * 0.5f);
            atmosphere.height = radius * CoronaHeight;
            atmosphere.lowerColor = atmColor;
            atmosphere.upperColor = atmColor;
            atmosphere.skyColor = atmColor;

            rp.atmosphere = &atmosphere;
        }
        else
#endif
        rp.atmosphere = NULL;

@JiliTheSpaceboy
Copy link
Contributor

That's how it looks with stars as scaled disks (in this mode the quad size is much smaller):
image

What would this look like if used with a normal brightness star texture?

@375gnu
Copy link
Member Author

375gnu commented Jan 1, 2024

What would this look like if used with a normal brightness star texture?

image

or even
image

@JiliTheSpaceboy
Copy link
Contributor

JiliTheSpaceboy commented Jan 1, 2024

What would this look like if used with a normal brightness star texture?

image

or even

image

Yup that looks lovely. Feels like we can actually use normal brightness textures in default now.

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.

None yet

4 participants