Changeset View
Standalone View
source/graphics/MiniMapTexture.cpp
Show All 24 Lines | |||||
#include "graphics/ShaderManager.h" | #include "graphics/ShaderManager.h" | ||||
#include "graphics/ShaderProgramPtr.h" | #include "graphics/ShaderProgramPtr.h" | ||||
#include "graphics/Terrain.h" | #include "graphics/Terrain.h" | ||||
#include "graphics/TerrainTextureEntry.h" | #include "graphics/TerrainTextureEntry.h" | ||||
#include "graphics/TerrainTextureManager.h" | #include "graphics/TerrainTextureManager.h" | ||||
#include "graphics/TerritoryTexture.h" | #include "graphics/TerritoryTexture.h" | ||||
#include "graphics/TextureManager.h" | #include "graphics/TextureManager.h" | ||||
#include "lib/bits.h" | #include "lib/bits.h" | ||||
#include "lib/hash.h" | |||||
#include "lib/timer.h" | #include "lib/timer.h" | ||||
#include "maths/MathUtil.h" | |||||
#include "maths/Vector2D.h" | #include "maths/Vector2D.h" | ||||
#include "ps/ConfigDB.h" | #include "ps/ConfigDB.h" | ||||
#include "ps/CStrInternStatic.h" | #include "ps/CStrInternStatic.h" | ||||
#include "ps/Filesystem.h" | #include "ps/Filesystem.h" | ||||
#include "ps/Game.h" | #include "ps/Game.h" | ||||
#include "ps/Profile.h" | |||||
#include "ps/VideoMode.h" | #include "ps/VideoMode.h" | ||||
#include "ps/World.h" | #include "ps/World.h" | ||||
#include "ps/XML/Xeromyces.h" | #include "ps/XML/Xeromyces.h" | ||||
#include "renderer/backend/IDevice.h" | #include "renderer/backend/IDevice.h" | ||||
#include "renderer/Renderer.h" | #include "renderer/Renderer.h" | ||||
#include "renderer/RenderingOptions.h" | #include "renderer/RenderingOptions.h" | ||||
#include "renderer/SceneRenderer.h" | #include "renderer/SceneRenderer.h" | ||||
#include "renderer/WaterManager.h" | #include "renderer/WaterManager.h" | ||||
#include "scriptinterface/Object.h" | #include "scriptinterface/Object.h" | ||||
#include "simulation2/Simulation2.h" | #include "simulation2/Simulation2.h" | ||||
#include "simulation2/components/ICmpMinimap.h" | #include "simulation2/components/ICmpMinimap.h" | ||||
#include "simulation2/components/ICmpRangeManager.h" | #include "simulation2/components/ICmpRangeManager.h" | ||||
#include "simulation2/system/ParamNode.h" | #include "simulation2/system/ParamNode.h" | ||||
#include <algorithm> | |||||
#include <array> | |||||
#include <cmath> | #include <cmath> | ||||
namespace | namespace | ||||
{ | { | ||||
// Set max drawn entities to 64K / 4 for now, which is more than enough. | // Set max drawn entities to 64K / 4 for now, which is more than enough. | ||||
// 4 is the number of vertices per entity. | // 4 is the number of vertices per entity. | ||||
// TODO: we should be cleverer about drawing them to reduce clutter, | // TODO: we should be cleverer about drawing them to reduce clutter, | ||||
// f.e. use instancing. | // f.e. use instancing. | ||||
const size_t MAX_ENTITIES_DRAWN = 65536 / 4; | constexpr size_t MAX_ENTITIES_DRAWN = 65536 / 4; | ||||
const size_t MAX_ICON_COUNT = 128; | constexpr size_t MAX_ICON_COUNT = 128; | ||||
constexpr size_t MAX_UNIQUE_ICON_COUNT = 64; | |||||
constexpr size_t ICON_COMBINING_GRID_SIZE = 10; | |||||
const size_t FINAL_TEXTURE_SIZE = 512; | constexpr size_t FINAL_TEXTURE_SIZE = 512; | ||||
unsigned int ScaleColor(unsigned int color, float x) | unsigned int ScaleColor(unsigned int color, float x) | ||||
{ | { | ||||
unsigned int r = unsigned(float(color & 0xff) * x); | unsigned int r = unsigned(float(color & 0xff) * x); | ||||
unsigned int g = unsigned(float((color >> 8) & 0xff) * x); | unsigned int g = unsigned(float((color >> 8) & 0xff) * x); | ||||
unsigned int b = unsigned(float((color >> 16) & 0xff) * x); | unsigned int b = unsigned(float((color >> 16) & 0xff) * x); | ||||
return (0xff000000 | b | g << 8 | r << 16); | return (0xff000000 | b | g << 8 | r << 16); | ||||
} | } | ||||
▲ Show 20 Lines • Show All 87 Lines • ▼ Show 20 Lines | for (const CVector2D& offset : offsets) | ||||
(*attrPos)[0] = v.position.X + offset.X; | (*attrPos)[0] = v.position.X + offset.X; | ||||
(*attrPos)[1] = v.position.Y + offset.Y; | (*attrPos)[1] = v.position.Y + offset.Y; | ||||
++attrPos; | ++attrPos; | ||||
} | } | ||||
} | } | ||||
} // anonymous namespace | } // anonymous namespace | ||||
size_t CMiniMapTexture::CellIconKeyHash::operator()( | |||||
const CellIconKey& key) const | |||||
{ | |||||
size_t seed = 0; | |||||
Stan: Inconsistent use of std::size_t | |||||
Done Inline ActionsCopy pasted from Texturemanager.cpp. vladislavbelov: Copy pasted from `Texturemanager.cpp`. | |||||
hash_combine(seed, key.path); | |||||
hash_combine(seed, key.r); | |||||
hash_combine(seed, key.g); | |||||
hash_combine(seed, key.b); | |||||
return seed; | |||||
} | |||||
bool CMiniMapTexture::CellIconKeyEqual::operator()( | |||||
const CellIconKey& lhs, const CellIconKey& rhs) const | |||||
{ | |||||
return | |||||
lhs.path == rhs.path && | |||||
lhs.r == rhs.r && | |||||
lhs.g == rhs.g && | |||||
lhs.b == rhs.b; | |||||
} | |||||
CMiniMapTexture::CMiniMapTexture(CSimulation2& simulation) | CMiniMapTexture::CMiniMapTexture(CSimulation2& simulation) | ||||
: m_Simulation(simulation), m_IndexArray(false), | : m_Simulation(simulation), m_IndexArray(false), | ||||
m_VertexArray(Renderer::Backend::IBuffer::Type::VERTEX, true), | m_VertexArray(Renderer::Backend::IBuffer::Type::VERTEX, true), | ||||
m_InstanceVertexArray(Renderer::Backend::IBuffer::Type::VERTEX, false) | m_InstanceVertexArray(Renderer::Backend::IBuffer::Type::VERTEX, false) | ||||
{ | { | ||||
// Register Relax NG validator. | // Register Relax NG validator. | ||||
CXeromyces::AddValidator(g_VFS, "pathfinder", "simulation/data/pathfinder.rng"); | CXeromyces::AddValidator(g_VFS, "pathfinder", "simulation/data/pathfinder.rng"); | ||||
▲ Show 20 Lines • Show All 229 Lines • ▼ Show 20 Lines | void CMiniMapTexture::RenderFinalTexture( | ||||
const double currentTime = timer_Time(); | const double currentTime = timer_Time(); | ||||
const bool doUpdate = (currentTime - m_LastFinalTextureUpdate > 0.5) || m_FinalTextureDirty; | const bool doUpdate = (currentTime - m_LastFinalTextureUpdate > 0.5) || m_FinalTextureDirty; | ||||
if (doUpdate) | if (doUpdate) | ||||
m_LastFinalTextureUpdate = currentTime; | m_LastFinalTextureUpdate = currentTime; | ||||
else | else | ||||
return; | return; | ||||
m_FinalTextureDirty = false; | m_FinalTextureDirty = false; | ||||
PROFILE3("Render minimap texture"); | |||||
Done Inline ActionsThat's good. Maybe we should have a ticket for those. Stan: That's good. Maybe we should have a ticket for those. | |||||
GPU_SCOPED_LABEL(deviceCommandContext, "Render minimap texture"); | GPU_SCOPED_LABEL(deviceCommandContext, "Render minimap texture"); | ||||
deviceCommandContext->SetFramebuffer(m_FinalTextureFramebuffer.get()); | deviceCommandContext->SetFramebuffer(m_FinalTextureFramebuffer.get()); | ||||
const SViewPort oldViewPort = g_Renderer.GetViewport(); | const SViewPort oldViewPort = g_Renderer.GetViewport(); | ||||
const SViewPort viewPort = { 0, 0, FINAL_TEXTURE_SIZE, FINAL_TEXTURE_SIZE }; | const SViewPort viewPort = { 0, 0, FINAL_TEXTURE_SIZE, FINAL_TEXTURE_SIZE }; | ||||
g_Renderer.SetViewport(viewPort); | g_Renderer.SetViewport(viewPort); | ||||
CmpPtr<ICmpRangeManager> cmpRangeManager(m_Simulation, SYSTEM_ENTITY); | CmpPtr<ICmpRangeManager> cmpRangeManager(m_Simulation, SYSTEM_ENTITY); | ||||
▲ Show 20 Lines • Show All 89 Lines • ▼ Show 20 Lines | void CMiniMapTexture::RenderFinalTexture( | ||||
// additional space in the vertex buffer. So we assume that we don't need | // additional space in the vertex buffer. So we assume that we don't need | ||||
// to change an entity size so often. | // to change an entity size so often. | ||||
// Radius with instancing is lower because an entity has a more round shape. | // Radius with instancing is lower because an entity has a more round shape. | ||||
const float entityRadius = static_cast<float>(m_MapSize) / 128.0f * (m_UseInstancing ? 5.0 : 6.0f); | const float entityRadius = static_cast<float>(m_MapSize) / 128.0f * (m_UseInstancing ? 5.0 : 6.0f); | ||||
if (doUpdate) | if (doUpdate) | ||||
{ | { | ||||
m_Icons.clear(); | m_Icons.clear(); | ||||
m_IconsCache.clear(); | |||||
CSimulation2::InterfaceList ents = m_Simulation.GetEntitiesWithInterface(IID_Minimap); | CSimulation2::InterfaceList ents = m_Simulation.GetEntitiesWithInterface(IID_Minimap); | ||||
VertexArrayIterator<float[2]> attrPos = m_AttributePos.GetIterator<float[2]>(); | VertexArrayIterator<float[2]> attrPos = m_AttributePos.GetIterator<float[2]>(); | ||||
VertexArrayIterator<u8[4]> attrColor = m_AttributeColor.GetIterator<u8[4]>(); | VertexArrayIterator<u8[4]> attrColor = m_AttributeColor.GetIterator<u8[4]>(); | ||||
m_EntitiesDrawn = 0; | m_EntitiesDrawn = 0; | ||||
MinimapUnitVertex v; | MinimapUnitVertex v; | ||||
Show All 40 Lines | for (CSimulation2::InterfaceList::const_iterator it = ents.begin(); it != ents.end(); ++it) | ||||
{ | { | ||||
AddEntity(v, attrColor, attrPos, entityRadius, m_UseInstancing); | AddEntity(v, attrColor, attrPos, entityRadius, m_UseInstancing); | ||||
++m_EntitiesDrawn; | ++m_EntitiesDrawn; | ||||
} | } | ||||
if (!iconsEnabled || !cmpMinimap->HasIcon()) | if (!iconsEnabled || !cmpMinimap->HasIcon()) | ||||
continue; | continue; | ||||
if (m_Icons.size() < MAX_ICON_COUNT) | const CellIconKey key{ | ||||
cmpMinimap->GetIconPath(), v.r, v.g, v.b}; | |||||
const u16 gridX = Clamp<u16>( | |||||
Done Inline Actionsu16 cannot be less than zero, std::min is sufficient. phosit: u16 cannot be less than zero, std::min is sufficient. | |||||
Done Inline ActionsActually I've mistaken here, it should be: const u16 gridX = static_cast<u16>(Clamp<int>( (v.position.X * invTileMapSize) * ICON_COMBINING_GRID_SIZE, 0, ICON_COMBINING_GRID_SIZE - 1)); vladislavbelov: Actually I've mistaken here, it should be:
```lang=cpp
const u16 gridX = static_cast<u16>… | |||||
(v.position.X * invTileMapSize) * ICON_COMBINING_GRID_SIZE, 0, ICON_COMBINING_GRID_SIZE - 1); | |||||
const u16 gridY = Clamp<u16>( | |||||
Done Inline Actionsi think std::clamp is suported by all compilers phosit: i think std::clamp is suported by all compilers | |||||
Done Inline ActionsVery slow on MSVC. like 10 times slower. Stan: Very slow on MSVC. like 10 times slower. | |||||
(v.position.Y * invTileMapSize) * ICON_COMBINING_GRID_SIZE, 0, ICON_COMBINING_GRID_SIZE - 1); | |||||
CellIcon icon{ | |||||
gridX, gridY, cmpMinimap->GetIconSize() * iconsSizeScale * 0.5f, v.position}; | |||||
if (m_IconsCache.find(key) == m_IconsCache.end() && m_IconsCache.size() >= MAX_UNIQUE_ICON_COUNT) | |||||
Done Inline ActionsWhy do you save the iterator in a variable? it is never used again. phosit: Why do you save the iterator in a variable? it is never used again. | |||||
Done Inline ActionsIt was used. vladislavbelov: It was used. | |||||
{ | |||||
Done Inline ActionsNo braces ? Stan: No braces ? | |||||
Done Inline ActionsNo need here. vladislavbelov: No need here. | |||||
iconsCountOverflow = true; | |||||
} | |||||
else | |||||
{ | |||||
m_IconsCache[key].emplace_back(std::move(icon)); | |||||
} | |||||
} | |||||
} | |||||
} | |||||
// We need to combine too close icons with the same path, we use a grid for | |||||
// that. But to save some allocations and space we store only the current | |||||
// row. | |||||
struct Cell | |||||
{ | |||||
u32 count; | |||||
float maxHalfSize; | |||||
CVector2D averagePosition; | |||||
}; | |||||
std::array<Cell, ICON_COMBINING_GRID_SIZE> gridRow; | |||||
for (auto& [key, icons] : m_IconsCache) | |||||
Done Inline ActionsInclude array. Stan: Include array. | |||||
{ | { | ||||
CTexturePtr texture = g_Renderer.GetTextureManager().CreateTexture( | CTexturePtr texture = g_Renderer.GetTextureManager().CreateTexture( | ||||
CTextureProperties(cmpMinimap->GetIconPath())); | CTextureProperties(key.path)); | ||||
const CColor color(v.r / 255.0f, v.g / 255.0f, v.b / 255.0f, iconsOpacity); | const CColor color(key.r / 255.0f, key.g / 255.0f, key.b / 255.0f, iconsOpacity); | ||||
Done Inline ActionsWhy no CColor in key? Stan: Why no CColor in key? | |||||
Done Inline ActionsLess space, u8 * 3 = 3 bytes, float * 4 = 16 bytes. vladislavbelov: Less space, `u8 * 3 = 3 bytes`, `float * 4 = 16 bytes`. | |||||
std::sort(icons.begin(), icons.end(), | |||||
[](const CellIcon& lhs, const CellIcon& rhs) -> bool | |||||
Done Inline ActionsAlgorithm Stan: Algorithm | |||||
{ | |||||
if (lhs.gridY != rhs.gridY) | |||||
return lhs.gridY < rhs.gridY; | |||||
return lhs.gridX < rhs.gridX; | |||||
}); | |||||
for (auto beginIt = icons.begin(); beginIt != icons.end();) | |||||
{ | |||||
Done Inline ActionsIf you use iterators it would be obvious: the variable is never used as a intager; the variable is never used to index another range. phosit: If you use iterators it would be obvious: the variable is never used as a intager; the variable… | |||||
Done Inline ActionsIterators should work. Also ++beginIndex shouldn't be there. vladislavbelov: Iterators should work. Also `++beginIndex` shouldn't be there. | |||||
auto endIt = std::next(beginIt); | |||||
while (endIt != icons.end() && beginIt->gridY == endIt->gridY) | |||||
++endIt; | |||||
Done Inline Actionsthis is a std::find_if: find the first element which differs from the begin. phosit: this is a `std::find_if`: find the first element which differs from the begin. | |||||
Done Inline ActionsSimple loop seems cleaner in that case: while (endIt != icons.end() && beginIt->gridY == endIt->gridY) ++endIt; // vs endIt = std::find_if(icons.begin(), icons.end(), [beginIt](const CellIcon& icon) { return beginIt->gridY != icon.gridY; }); vladislavbelov: Simple loop seems cleaner in that case:
```lang=cpp
while (endIt != icons.end() && beginIt… | |||||
Not Done Inline Actionsthis comment is only my opinnion. generaly are std algorithms more expresive, if i read the function name i know (abstractly) what it does:
while and for on the other hand are used for all of the above and more. There are also valid use-cases. phosit: this comment is only my opinnion.
find is easier to reason about since it is more human… | |||||
Not Done Inline ActionsI don't necessarily disagree with you here, but I'll point out that this is a double edged sword:
In this particular instance I might agree, but I think it's not really workable as a general rule... wraitii: I don't necessarily disagree with you here, but I'll point out that this is a double edged… | |||||
gridRow.fill({0, 0.0f, {}}); | |||||
for (; beginIt != endIt; ++beginIt) | |||||
Done Inline ActionsgridRow.fill({0, 0.0f, {}}) phosit: `gridRow.fill({0, 0.0f, {}})` | |||||
{ | |||||
Cell& cell = gridRow[beginIt->gridX]; | |||||
const float previousPositionWeight = static_cast<float>(cell.count) / (cell.count + 1); | |||||
cell.averagePosition = cell.averagePosition * previousPositionWeight + beginIt->worldPosition / static_cast<float>(cell.count + 1); | |||||
Done Inline ActionsWhy not CVector2D() ? Stan: Why not CVector2D() ? | |||||
Done Inline ActionsI have no strong opinion on that yet, might be CVector2D(). vladislavbelov: I have no strong opinion on that yet, might be `CVector2D()`. | |||||
cell.maxHalfSize = std::max(cell.maxHalfSize, beginIt->halfSize); | |||||
++cell.count; | |||||
Done Inline ActionsIterating between two iterators is a std::for_each phosit: Iterating between two iterators is a `std::for_each` | |||||
Done Inline ActionsThe loop seems cleaner, because you also don't need to advance beginIt. vladislavbelov: The loop seems cleaner, because you also don't need to advance `beginIt`. | |||||
} | |||||
for (const Cell& cell : gridRow) | |||||
{ | |||||
if (cell.count == 0) | |||||
continue; | |||||
if (m_Icons.size() < MAX_ICON_COUNT) | |||||
{ | |||||
m_Icons.emplace_back(Icon{ | m_Icons.emplace_back(Icon{ | ||||
std::move(texture), color, v.position, cmpMinimap->GetIconSize() * iconsSizeScale * 0.5f}); | texture, color, cell.averagePosition, cell.maxHalfSize}); | ||||
} | } | ||||
else | else | ||||
{ | |||||
iconsCountOverflow = true; | iconsCountOverflow = true; | ||||
} | } | ||||
} | } | ||||
} | } | ||||
} | |||||
if (iconsCountOverflow) | if (iconsCountOverflow) | ||||
LOGWARNING("Too many minimap icons to draw: %zu/%zu", m_Icons.size(), MAX_ICON_COUNT); | LOGWARNING("Too many minimap icons to draw."); | ||||
Done Inline Actions. Stan: . | |||||
Done Inline ActionsIt's intended as the number doesn't really help, because it's a compiled constant. vladislavbelov: It's intended as the number doesn't really help, because it's a compiled constant. | |||||
Done Inline ActionsMeant it's missing a final period. :) Stan: Meant it's missing a final period. :)
Wonder if it's worth showing a warning anymore. Could be… | |||||
Done Inline ActionsAh, thanks. vladislavbelov: Ah, thanks.
I think yes, because it still can overflow. | |||||
// Add the pinged vertices at the end, so they are drawn on top | // Add the pinged vertices at the end, so they are drawn on top | ||||
for (const MinimapUnitVertex& vertex : pingingVertices) | for (const MinimapUnitVertex& vertex : pingingVertices) | ||||
{ | { | ||||
AddEntity(vertex, attrColor, attrPos, entityRadius, m_UseInstancing); | AddEntity(vertex, attrColor, attrPos, entityRadius, m_UseInstancing); | ||||
++m_EntitiesDrawn; | ++m_EntitiesDrawn; | ||||
} | } | ||||
▲ Show 20 Lines • Show All 124 Lines • Show Last 20 Lines |
Inconsistent use of std::size_t