From f65eaaba4d5afe5dbceec0e90e1541725fcec058 Mon Sep 17 00:00:00 2001 From: Malin Ejdbo Date: Fri, 5 Feb 2021 14:14:13 +0100 Subject: [PATCH] Address OpenSpace PR comments --- modules/base/rendering/renderablemodel.cpp | 14 +-- modules/base/shaders/model_fs.glsl | 94 +++++++++---------- .../rendering/renderablemodelprojection.cpp | 16 ++-- 3 files changed, 61 insertions(+), 63 deletions(-) diff --git a/modules/base/rendering/renderablemodel.cpp b/modules/base/rendering/renderablemodel.cpp index 911712192c..8ddb99f7b9 100644 --- a/modules/base/rendering/renderablemodel.cpp +++ b/modules/base/rendering/renderablemodel.cpp @@ -305,22 +305,22 @@ RenderableModel::RenderableModel(const ghoul::Dictionary& dictionary) )); } - if (geometries.size() > 0) { + if (!geometries.empty()) { ghoul::modelgeometry::ModelGeometry combinedGeometry = std::move(*geometries[0].release()); // Combine all models into one ModelGeometry for (unsigned int i = 1; i < geometries.size(); ++i) { - for (unsigned int m = 0; m < geometries[i]->meshes().size(); ++m) { + for (const ghoul::io::ModelMesh& mesh : geometries[i]->meshes()) { combinedGeometry.meshes().push_back( - std::move(geometries[i]->meshes()[m]) + std::move(mesh) ); } - for (unsigned int t = 0; t < geometries[i]->textureStorage().size(); ++t) { - combinedGeometry.textureStorage().push_back( - std::move(geometries[i]->textureStorage()[t]) - ); + for (const ghoul::modelgeometry::ModelGeometry::TextureEntry& texture + : geometries[i]->textureStorage()) + { + combinedGeometry.textureStorage().push_back(std::move(texture)); } } _geometry = std::make_unique( diff --git a/modules/base/shaders/model_fs.glsl b/modules/base/shaders/model_fs.glsl index 8f0989cd57..2e5a5fbd80 100644 --- a/modules/base/shaders/model_fs.glsl +++ b/modules/base/shaders/model_fs.glsl @@ -58,33 +58,33 @@ uniform float opacity = 1.0; Fragment getFragment() { - // Render invisible mesh with flashy procedural material - if(use_forced_color) { - Fragment frag; + // Render invisible mesh with flashy procedural material + if (use_forced_color) { + Fragment frag; - vec3 adjustedPos = floor(vs_positionCameraSpace.xyz * 3.0); - float chessboard = adjustedPos.x + adjustedPos.y + adjustedPos.z; - chessboard = fract(chessboard * 0.5); - chessboard *= 2; - // Pink and complementary green in a chessboard pattern - frag.color.rgb = mix(vec3(1.0, 0.0, 0.8), vec3(0.0, 1.0, 0.2), chessboard); + vec3 adjustedPos = floor(vs_positionCameraSpace.xyz * 3.0); + float chessboard = adjustedPos.x + adjustedPos.y + adjustedPos.z; + chessboard = fract(chessboard * 0.5); + chessboard *= 2; + // Pink and complementary green in a chessboard pattern + frag.color.rgb = mix(vec3(1.0, 0.0, 0.8), vec3(0.0, 1.0, 0.2), chessboard); - frag.color.a = opacity; - frag.depth = vs_screenSpaceDepth; - frag.gPosition = vs_positionCameraSpace; - frag.gNormal = vec4(vs_normalViewSpace, 0.0); - frag.disableLDR2HDR = true; + frag.color.a = opacity; + frag.depth = vs_screenSpaceDepth; + frag.gPosition = vs_positionCameraSpace; + frag.gNormal = vec4(vs_normalViewSpace, 0.0); + frag.disableLDR2HDR = true; - return frag; - } + return frag; + } - vec3 diffuseAlbedo; - if (has_texture_diffuse) { - diffuseAlbedo = texture(texture_diffuse, vs_st).rgb; - } - else { - diffuseAlbedo = color_diffuse; - } + vec3 diffuseAlbedo; + if (has_texture_diffuse) { + diffuseAlbedo = texture(texture_diffuse, vs_st).rgb; + } + else { + diffuseAlbedo = color_diffuse; + } if (opacity == 0.0) { discard; @@ -94,32 +94,32 @@ Fragment getFragment() { if (performShading) { - vec3 specularAlbedo; - if (has_texture_specular) { - specularAlbedo = texture(texture_specular, vs_st).rgb; - } - else { - if(has_color_specular) { - specularAlbedo = color_specular; - } - else { - specularAlbedo = vec3(1.0); - } - } + vec3 specularAlbedo; + if (has_texture_specular) { + specularAlbedo = texture(texture_specular, vs_st).rgb; + } + else { + if (has_color_specular) { + specularAlbedo = color_specular; + } + else { + specularAlbedo = vec3(1.0); + } + } // Some of these values could be passed in as uniforms const vec3 lightColorAmbient = vec3(1.0); const vec3 lightColor = vec3(1.0); vec3 n; - if(has_texture_normal) { - vec3 normalAlbedo = texture(texture_normal, vs_st).rgb; - normalAlbedo = normalize(normalAlbedo * 2.0 - 1.0); - n = normalize(TBN * normalAlbedo); - } - else { - n = normalize(vs_normalViewSpace); - } + if (has_texture_normal) { + vec3 normalAlbedo = texture(texture_normal, vs_st).rgb; + normalAlbedo = normalize(normalAlbedo * 2.0 - 1.0); + n = normalize(TBN * normalAlbedo); + } + else { + n = normalize(vs_normalViewSpace); + } vec3 c = normalize(vs_positionCameraSpace.xyz); @@ -133,9 +133,9 @@ Fragment getFragment() { float specularCosineFactor = dot(c,r); const float specularPower = 100.0; - vec3 diffuseColor = - diffuseIntensity * lightColor * diffuseAlbedo * - max(diffuseCosineFactor, 0); + vec3 diffuseColor = + diffuseIntensity * lightColor * diffuseAlbedo * + max(diffuseCosineFactor, 0); vec3 specularColor = specularIntensity * lightColor * specularAlbedo * @@ -167,4 +167,4 @@ Fragment getFragment() { frag.disableLDR2HDR = true; return frag; -} \ No newline at end of file +} diff --git a/modules/spacecraftinstruments/rendering/renderablemodelprojection.cpp b/modules/spacecraftinstruments/rendering/renderablemodelprojection.cpp index 574539bbf6..de6edcccf4 100644 --- a/modules/spacecraftinstruments/rendering/renderablemodelprojection.cpp +++ b/modules/spacecraftinstruments/rendering/renderablemodelprojection.cpp @@ -93,7 +93,7 @@ documentation::Documentation RenderableModelProjection::Documentation() { KeyGeomModelFile, new OrVerifier({ new StringVerifier, new StringListVerifier }), Optional::No, - "The file or files that that is used for rendering this model" + "The file or files that are used for rendering of this model" }, { keyProjection, @@ -160,16 +160,14 @@ RenderableModelProjection::RenderableModelProjection(const ghoul::Dictionary& di // Combine all models into one ModelGeometry for (unsigned int i = 1; i < geometries.size(); ++i) { - for (unsigned int m = 0; m < geometries[i]->meshes().size(); ++m) { - combinedGeometry.meshes().push_back( - std::move(geometries[i]->meshes()[m]) - ); + for (const ghoul::io::ModelMesh& mesh : geometries[i]->meshes()) { + combinedGeometry.meshes().push_back(std::move(mesh)); } - for (unsigned int t = 0; t < geometries[i]->textureStorage().size(); ++t) { - combinedGeometry.textureStorage().push_back( - std::move(geometries[i]->textureStorage()[t]) - ); + for (const ghoul::modelgeometry::ModelGeometry::TextureEntry& texture + : geometries[i]->textureStorage()) + { + combinedGeometry.textureStorage().push_back(std::move(texture)); } } _geometry = std::make_unique(