From e623e3863876b528f5bac4e742ea9eef46f6917f Mon Sep 17 00:00:00 2001 From: benpm Date: Thu, 17 Jul 2025 12:33:40 -0600 Subject: [PATCH] applying more suggestions from code review --- .../shaders/localrenderer_vs.glsl | 6 +- .../globebrowsing/shaders/renderer_fs.glsl | 13 +- modules/globebrowsing/src/renderableglobe.cpp | 116 +++++++----------- modules/globebrowsing/src/ringscomponent.cpp | 20 +-- modules/globebrowsing/src/ringscomponent.h | 14 +-- 5 files changed, 70 insertions(+), 99 deletions(-) diff --git a/modules/globebrowsing/shaders/localrenderer_vs.glsl b/modules/globebrowsing/shaders/localrenderer_vs.glsl index e5fa5724a2..33e66081b0 100644 --- a/modules/globebrowsing/shaders/localrenderer_vs.glsl +++ b/modules/globebrowsing/shaders/localrenderer_vs.glsl @@ -37,7 +37,6 @@ out vec4 fs_position; out vec3 ellipsoidNormalCameraSpace; out vec3 levelWeights; out vec3 positionCameraSpace; -out vec3 positionWorldSpace; out vec3 posObjSpace; out vec3 normalObjSpace; @@ -56,6 +55,11 @@ uniform dmat4 inverseViewTransform; out vec4 shadowCoords; #endif // SHADOW_MAPPING_ENABLED +#if USE_ECLIPSE_SHADOWS + // Position in world space + out vec3 positionWorldSpace; +#endif // USE_ECLIPSE_SHADOWS + uniform mat4 projectionTransform; // Input points in camera space uniform vec3 p00; diff --git a/modules/globebrowsing/shaders/renderer_fs.glsl b/modules/globebrowsing/shaders/renderer_fs.glsl index eb68ae1587..7862d44404 100644 --- a/modules/globebrowsing/shaders/renderer_fs.glsl +++ b/modules/globebrowsing/shaders/renderer_fs.glsl @@ -63,18 +63,17 @@ uniform float ambientIntensity; #if SHADOW_MAPPING_ENABLED #if USE_RING_SHADOWS +// Fragment position in object space +in vec3 posObjSpace; + // Color of the rings uniform sampler1D ringTextureColor; // Transparency of the rings uniform sampler1D ringTextureTransparency; uniform vec2 textureOffset; -uniform vec3 sunPositionWorld; // NEW: world coordinates -uniform vec3 camPositionWorld; // NEW: world coordinates #endif // USE_RING_SHADOWS #endif // SHADOW_MAPPING_ENABLED -in vec3 posObjSpace; // Fragment position in object space - #if USE_ECLIPSE_SHADOWS #define NSEclipseShadowsMinusOne #{nEclipseShadows} @@ -322,11 +321,9 @@ Fragment getFragment() { // Increase the shadow darkness factor with low angle to simulate the light having // to pass through more material - float angleFactor = clamp(abs(-dot(ringPlaneNormal, surfaceToSun)) / 2.0, 0.0, 0.3); - + float angleFactor = clamp(abs(dot(ringPlaneNormal, surfaceToSun)) / 2.0, 0.0, 0.3); // Calculate shadow factor based on ring opacity - shadow = ringOpacity; - shadow = clamp(shadow + angleFactor, 0.05, 1.0); + shadow = clamp(ringOpacity + angleFactor, 0.05, 1.0); lightColor = texture(ringTextureColor, texCoord).rgb; } } diff --git a/modules/globebrowsing/src/renderableglobe.cpp b/modules/globebrowsing/src/renderableglobe.cpp index 2af7c6c273..5002ed349b 100644 --- a/modules/globebrowsing/src/renderableglobe.cpp +++ b/modules/globebrowsing/src/renderableglobe.cpp @@ -773,9 +773,11 @@ RenderableGlobe::RenderableGlobe(const ghoul::Dictionary& dictionary) _ringsComponent = std::make_unique(*p.rings); _ringsComponent->setParentFadeable(this); _ringsComponent->initialize(); + _ringsComponent->onReadinessChange([this]() { + _shadersNeedRecompilation = true; + }); addPropertySubOwner(_ringsComponent.get()); - - // Set up notification for shader recompilation when rings state changes + auto* enabledProperty = static_cast( _ringsComponent->property("Enabled") ); @@ -1257,32 +1259,11 @@ void RenderableGlobe::renderChunks(const RenderData& data, RendererTasks&, } using namespace layers; - bool nightLayersActive = + const bool nightLayersActive = !_layerManager.layerGroup(Group::ID::NightLayers).activeLayers().empty(); - bool waterLayersActive = + const bool waterLayersActive = !_layerManager.layerGroup(Group::ID::WaterMasks).activeLayers().empty(); - if (nightLayersActive || waterLayersActive || _performShading) { - const glm::dvec3 directionToSunWorldSpace = - directionToLightSource(data.modelTransform.translation, _lightSourceNode); - - const glm::vec3 directionToSunCameraSpace = glm::vec3(viewTransform * - glm::dvec4(directionToSunWorldSpace, 0)); - const glm::vec3 directionToSunObjSpace = glm::vec3(_cachedInverseModelTransform * - glm::dvec4(directionToSunWorldSpace, 0)); - - using IgnoreError = ghoul::opengl::ProgramObject::IgnoreError; - _globalRenderer.program->setIgnoreUniformLocationError(IgnoreError::Yes); - _globalRenderer.program->setUniform( - "lightDirectionCameraSpace", - -glm::normalize(directionToSunCameraSpace) - ); - _globalRenderer.program->setUniform( - "lightDirectionObjSpace", - -glm::normalize(directionToSunObjSpace) - ); - } - if (_useAccurateNormals && hasHeightLayer) { // Apply an extra scaling to the height if the object is scaled _localRenderer.program->setUniform( @@ -1293,23 +1274,17 @@ void RenderableGlobe::renderChunks(const RenderData& data, RendererTasks&, ); } - using namespace layers; - nightLayersActive = - !_layerManager.layerGroup(Group::ID::NightLayers).activeLayers().empty(); - waterLayersActive = - !_layerManager.layerGroup(Group::ID::WaterMasks).activeLayers().empty(); - + // Light direction uniforms, only used in fragment shader if (nightLayersActive || waterLayersActive || _performShading) { const glm::dvec3 directionToSunWorldSpace = directionToLightSource(data.modelTransform.translation, _lightSourceNode); + const glm::vec3 directionToSunCameraSpace(viewTransform * + glm::dvec4(directionToSunWorldSpace, 0.0)); + const glm::vec3 directionToSunObjSpace(_cachedInverseModelTransform * + glm::dvec4(directionToSunWorldSpace, 0.0)); - const glm::vec3 directionToSunCameraSpace = glm::vec3(viewTransform * - glm::dvec4(directionToSunWorldSpace, 0)); - const glm::vec3 directionToSunObjSpace = glm::vec3(_cachedInverseModelTransform * - glm::dvec4(directionToSunWorldSpace, 0)); - + // Set the light direction uniforms for local renderer using IgnoreError = ghoul::opengl::ProgramObject::IgnoreError; - _localRenderer.program->setIgnoreUniformLocationError(IgnoreError::Yes); _localRenderer.program->setUniform( "lightDirectionCameraSpace", -glm::normalize(directionToSunCameraSpace) @@ -1318,7 +1293,6 @@ void RenderableGlobe::renderChunks(const RenderData& data, RendererTasks&, "lightDirectionObjSpace", -glm::normalize(directionToSunObjSpace) ); - _localRenderer.program->setIgnoreUniformLocationError(IgnoreError::Yes); } int globalCount = 0; @@ -1477,7 +1451,9 @@ void RenderableGlobe::renderChunkGlobally(const Chunk& chunk, const RenderData& if (_eclipseShadowsEnabled && !_ellipsoid.shadowConfigurationArray().empty()) { calculateEclipseShadows(program, data, ShadowCompType::GLOBAL_SHADOW); - } // Shadow Mapping + } + + // Shadow Mapping ghoul::opengl::TextureUnit shadowMapUnit; if (_shadowMappingProperties.shadowMapping && shadowData.shadowDepthTexture != 0) { // Adding the model transformation to the final shadow matrix so we have a @@ -1505,33 +1481,33 @@ void RenderableGlobe::renderChunkGlobally(const Chunk& chunk, const RenderData& ghoul::opengl::TextureUnit ringTextureColorUnit; ghoul::opengl::TextureUnit ringTextureTransparencyUnit; - if (_ringsComponent->ringTextureFwrd()) { + if (_ringsComponent->textureForwards()) { ringTextureFwrdUnit.activate(); - _ringsComponent->ringTextureFwrd()->bind(); + _ringsComponent->textureForwards()->bind(); program.setUniform("ringTextureFwrd", ringTextureFwrdUnit); } - if (_ringsComponent->ringTextureBckwrd()) { + if (_ringsComponent->textureBackwards()) { ringTextureBckwrdUnit.activate(); - _ringsComponent->ringTextureBckwrd()->bind(); + _ringsComponent->textureBackwards()->bind(); program.setUniform("ringTextureBckwrd", ringTextureBckwrdUnit); } - if (_ringsComponent->ringTextureUnlit()) { + if (_ringsComponent->textureUnlit()) { ringTextureUnlitUnit.activate(); - _ringsComponent->ringTextureUnlit()->bind(); + _ringsComponent->textureUnlit()->bind(); program.setUniform("ringTextureUnlit", ringTextureUnlitUnit); } - if (_ringsComponent->ringTextureColor()) { + if (_ringsComponent->textureColor()) { ringTextureColorUnit.activate(); - _ringsComponent->ringTextureColor()->bind(); + _ringsComponent->textureColor()->bind(); program.setUniform("ringTextureColor", ringTextureColorUnit); } - if (_ringsComponent->ringTextureTransparency()) { + if (_ringsComponent->textureTransparency()) { ringTextureTransparencyUnit.activate(); - _ringsComponent->ringTextureTransparency()->bind(); + _ringsComponent->textureTransparency()->bind(); program.setUniform("ringTextureTransparency", ringTextureTransparencyUnit); } @@ -1659,7 +1635,9 @@ void RenderableGlobe::renderChunkLocally(const Chunk& chunk, const RenderData& d if (_eclipseShadowsEnabled && !_ellipsoid.shadowConfigurationArray().empty()) { calculateEclipseShadows(program, data, ShadowCompType::LOCAL_SHADOW); - } // Shadow Mapping + } + + // Shadow Mapping ghoul::opengl::TextureUnit shadowMapUnit; if (_shadowMappingProperties.shadowMapping && shadowData.shadowDepthTexture != 0) { // Adding the model transformation to the final shadow matrix so we have a @@ -1687,33 +1665,33 @@ void RenderableGlobe::renderChunkLocally(const Chunk& chunk, const RenderData& d ghoul::opengl::TextureUnit ringTextureColorUnit; ghoul::opengl::TextureUnit ringTextureTransparencyUnit; - if (_ringsComponent->ringTextureFwrd()) { + if (_ringsComponent->textureForwards()) { ringTextureFwrdUnit.activate(); - _ringsComponent->ringTextureFwrd()->bind(); + _ringsComponent->textureForwards()->bind(); program.setUniform("ringTextureFwrd", ringTextureFwrdUnit); } - if (_ringsComponent->ringTextureBckwrd()) { + if (_ringsComponent->textureBackwards()) { ringTextureBckwrdUnit.activate(); - _ringsComponent->ringTextureBckwrd()->bind(); + _ringsComponent->textureBackwards()->bind(); program.setUniform("ringTextureBckwrd", ringTextureBckwrdUnit); } - if (_ringsComponent->ringTextureUnlit()) { + if (_ringsComponent->textureUnlit()) { ringTextureUnlitUnit.activate(); - _ringsComponent->ringTextureUnlit()->bind(); + _ringsComponent->textureUnlit()->bind(); program.setUniform("ringTextureUnlit", ringTextureUnlitUnit); } - if (_ringsComponent->ringTextureColor()) { + if (_ringsComponent->textureColor()) { ringTextureColorUnit.activate(); - _ringsComponent->ringTextureColor()->bind(); + _ringsComponent->textureColor()->bind(); program.setUniform("ringTextureColor", ringTextureColorUnit); } - if (_ringsComponent->ringTextureTransparency()) { + if (_ringsComponent->textureTransparency()) { ringTextureTransparencyUnit.activate(); - _ringsComponent->ringTextureTransparency()->bind(); + _ringsComponent->textureTransparency()->bind(); program.setUniform("ringTextureTransparency", ringTextureTransparencyUnit); } @@ -1830,17 +1808,6 @@ void RenderableGlobe::setCommonUniforms(ghoul::opengl::ProgramObject& programObj programObject.setUniform("tileDelta", TileDelta); } - // Set sunPositionWorld and camPositionWorld uniforms for ring shadow calculations - // Get Sun node position in world coordinates - glm::dvec3 sunPositionWorld = glm::dvec3(0.0); - SceneGraphNode* sunNode = sceneGraph()->sceneGraphNode("Sun"); - if (sunNode) { - sunPositionWorld = sunNode->worldPosition(); - } - // Camera position in world coordinates - glm::dvec3 camPositionWorld = data.camera.positionVec3(); - programObject.setUniform("sunPositionWorld", glm::vec3(sunPositionWorld)); - programObject.setUniform("camPositionWorld", glm::vec3(camPositionWorld)); programObject.setUniform("modelTransform", _cachedModelTransform); } @@ -1895,7 +1862,7 @@ void RenderableGlobe::recompileShaders() { } std::vector>& pairs = - preprocessingData.keyValuePairs; + preprocessingData.keyValuePairs; const bool hasHeightLayer = !_layerManager.layerGroup(layers::Group::ID::HeightLayers).activeLayers().empty(); @@ -1921,7 +1888,10 @@ void RenderableGlobe::recompileShaders() { pairs.emplace_back("defaultHeight", std::to_string(DefaultHeight)); // log if ring shadows are enabled - LINFO(std::format("Ring shadows enabled: {}", _shadowMappingProperties.shadowMapping && _ringsComponent)); + LINFO(std::format( + "Ring shadows enabled: {}", + _shadowMappingProperties.shadowMapping && _ringsComponent + )); // // Create dictionary from layerpreprocessing data diff --git a/modules/globebrowsing/src/ringscomponent.cpp b/modules/globebrowsing/src/ringscomponent.cpp index 70129bd96e..d1b5f21a54 100644 --- a/modules/globebrowsing/src/ringscomponent.cpp +++ b/modules/globebrowsing/src/ringscomponent.cpp @@ -441,35 +441,35 @@ void RingsComponent::draw(const RenderData& data, RenderPass renderPass, ringTextureFwrdUnit.activate(); _textureForwards->bind(); _shader->setUniform( - _uniformCacheAdvancedRings.ringTextureFwrd, + _uniformCacheAdvancedRings.textureForwards, ringTextureFwrdUnit ); ringTextureBckwrdUnit.activate(); _textureBackwards->bind(); _shader->setUniform( - _uniformCacheAdvancedRings.ringTextureBckwrd, + _uniformCacheAdvancedRings.textureBackwards, ringTextureBckwrdUnit ); ringTextureUnlitUnit.activate(); _textureUnlit->bind(); _shader->setUniform( - _uniformCacheAdvancedRings.ringTextureUnlit, + _uniformCacheAdvancedRings.textureUnlit, ringTextureUnlitUnit ); ringTextureColorUnit.activate(); _textureColor->bind(); _shader->setUniform( - _uniformCacheAdvancedRings.ringTextureColor, + _uniformCacheAdvancedRings.textureColor, ringTextureColorUnit ); ringTextureTransparencyUnit.activate(); _textureTransparency->bind(); _shader->setUniform( - _uniformCacheAdvancedRings.ringTextureTransparency, + _uniformCacheAdvancedRings.textureTransparency, ringTextureTransparencyUnit ); _shader->setUniform(_uniformCacheAdvancedRings.opacity, opacity()); @@ -876,23 +876,23 @@ double RingsComponent::size() const { return _size; } -ghoul::opengl::Texture* RingsComponent::ringTextureFwrd() const { +ghoul::opengl::Texture* RingsComponent::textureForwards() const { return _textureForwards.get(); } -ghoul::opengl::Texture* RingsComponent::ringTextureBckwrd() const { +ghoul::opengl::Texture* RingsComponent::textureBackwards() const { return _textureBackwards.get(); } -ghoul::opengl::Texture* RingsComponent::ringTextureUnlit() const { +ghoul::opengl::Texture* RingsComponent::textureUnlit() const { return _textureUnlit.get(); } -ghoul::opengl::Texture* RingsComponent::ringTextureColor() const { +ghoul::opengl::Texture* RingsComponent::textureColor() const { return _textureColor.get(); } -ghoul::opengl::Texture* RingsComponent::ringTextureTransparency() const { +ghoul::opengl::Texture* RingsComponent::textureTransparency() const { return _textureTransparency.get(); } diff --git a/modules/globebrowsing/src/ringscomponent.h b/modules/globebrowsing/src/ringscomponent.h index 8174e4a0ae..14f7e69e2e 100644 --- a/modules/globebrowsing/src/ringscomponent.h +++ b/modules/globebrowsing/src/ringscomponent.h @@ -79,11 +79,11 @@ public: void onReadinessChange(ReadinessChangeCallback callback); // Texture access methods for globe rendering - ghoul::opengl::Texture* ringTextureFwrd() const; - ghoul::opengl::Texture* ringTextureBckwrd() const; - ghoul::opengl::Texture* ringTextureUnlit() const; - ghoul::opengl::Texture* ringTextureColor() const; - ghoul::opengl::Texture* ringTextureTransparency() const; + ghoul::opengl::Texture* textureForwards() const; + ghoul::opengl::Texture* textureBackwards() const; + ghoul::opengl::Texture* textureUnlit() const; + ghoul::opengl::Texture* textureColor() const; + ghoul::opengl::Texture* textureTransparency() const; glm::vec2 textureOffset() const; glm::vec3 sunPositionObj() const; glm::vec3 camPositionObj() const; @@ -115,8 +115,8 @@ private: opacity ) _uniformCache; UniformCache(modelViewProjectionMatrix, textureOffset, colorFilterValue, nightFactor, - sunPosition, sunPositionObj, camPositionObj, ringTextureFwrd, ringTextureBckwrd, - ringTextureUnlit, ringTextureColor, ringTextureTransparency, shadowMatrix, + sunPosition, sunPositionObj, camPositionObj, textureForwards, textureBackwards, + textureUnlit, textureColor, textureTransparency, shadowMatrix, shadowMapTexture, zFightingPercentage, opacity ) _uniformCacheAdvancedRings; UniformCache(modelViewProjectionMatrix, textureOffset, ringTexture) _geomUniformCache;