Page MenuHomeWildfire Games
Differential D4702 Diff 20478 source/graphics/MiniMapTexture.cpp

Changeset View

Standalone View

View Options

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;
StanUnsubmitted
Done
Inline Actions

Inconsistent use of std::size_t

Stan: Inconsistent use of std::size_t
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Copy 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 Linesvoid 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");
StanUnsubmitted
Done
Inline Actions

That'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 Linesvoid 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>(
phositUnsubmitted
Done
Inline Actions

u16 cannot be less than zero, std::min is sufficient.

phosit: u16 cannot be less than zero, std::min is sufficient.
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Actually 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>(
phositUnsubmitted
Done
Inline Actions

i think std::clamp is suported by all compilers

phosit: i think std::clamp is suported by all compilers
StanUnsubmitted
Done
Inline Actions

Very 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 (auto it = m_IconsCache.find(key);
phositUnsubmitted
Done
Inline Actions

Why 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.
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

It was used.

vladislavbelov: It was used.
it == m_IconsCache.end() && m_IconsCache.size() >= MAX_UNIQUE_ICON_COUNT)
StanUnsubmitted
Done
Inline Actions

No braces ?

Stan: No braces ?
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

No 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;
StanUnsubmitted
Done
Inline Actions

Include array.

Stan: Include array.
for (auto& [key, icons] : m_IconsCache)
{ {
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);
StanUnsubmitted
Done
Inline Actions

Why no CColor in key?

Stan: Why no CColor in key?
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Less 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(),
StanUnsubmitted
Done
Inline Actions

Algorithm

Stan: Algorithm
[](const CellIcon& lhs, const CellIcon& rhs) -> bool
{
if (lhs.gridY != rhs.gridY)
return lhs.gridY < rhs.gridY;
return lhs.gridX < rhs.gridX;
});
for (size_t beginIndex = 0; beginIndex < icons.size(); ++beginIndex)
phositUnsubmitted
Done
Inline Actions

If you use iterators it would be obvious: the variable is never used as a intager; the variable is never used to index another range.
But there is the problem with overruning the end.

phosit: If you use iterators it would be obvious: the variable is never used as a intager; the variable…
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Iterators should work. Also ++beginIndex shouldn't be there.

vladislavbelov: Iterators should work. Also `++beginIndex` shouldn't be there.
{
size_t endIndex = beginIndex + 1;
while (endIndex < icons.size() && icons[beginIndex].gridY == icons[endIndex].gridY)
phositUnsubmitted
Done
Inline Actions

this 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.
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Simple 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…
phositUnsubmitted
Not Done
Inline Actions

this comment is only my opinnion.
find is easier to reason about since it is more human language:
"find the first element int the range of beginIt to icons.end() of which a member is not equal to the member in beginIt."
"as long as endIt is not equal to icons.end() and a member of endIt is equal to the member in beginIt, increment endIt."

generaly are std algorithms more expresive, if i read the function name i know (abstractly) what it does:

  • find: tryes to find an element.
  • accumulate: accumulates all the element.
  • replace: replace some elements in the range.
  • count: counts some elements in the range.
  • ...

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…
wraitiiUnsubmitted
Not Done
Inline Actions

I don't necessarily disagree with you here, but I'll point out that this is a double edged sword:

  • find, sure, it finds. Easy enough.
  • accumulate is quickly _way_ less obvious though.
  • transform is vague as f***

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…
++endIndex;
for (Cell& cell : gridRow)
phositUnsubmitted
Done
Inline Actions

gridRow.fill({0, 0.0f, {}})

phosit: `gridRow.fill({0, 0.0f, {}})`
{
cell.count = 0;
cell.maxHalfSize = 0.0f;
cell.averagePosition = CVector2D{};
StanUnsubmitted
Done
Inline Actions

Why not CVector2D() ?

Stan: Why not CVector2D() ?
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

I have no strong opinion on that yet, might be CVector2D().

vladislavbelov: I have no strong opinion on that yet, might be `CVector2D()`.
}
for (; beginIndex < endIndex; ++beginIndex)
phositUnsubmitted
Done
Inline Actions

Iterating between two iterators is a std::for_each

phosit: Iterating between two iterators is a `std::for_each`
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

The 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`.
{
Cell& cell = gridRow[icons[beginIndex].gridX];
const float previousPositionWeight = static_cast<float>(cell.count) / (cell.count + 1);
cell.averagePosition = cell.averagePosition * previousPositionWeight + icons[beginIndex].worldPosition / static_cast<float>(cell.count + 1);
cell.maxHalfSize = std::max(cell.maxHalfSize, icons[beginIndex].halfSize);
++cell.count;
}
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");
StanUnsubmitted
Done
Inline Actions

.

Stan: .
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

It'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.
StanUnsubmitted
Done
Inline Actions

Meant it's missing a final period. :)
Wonder if it's worth showing a warning anymore. Could be a loginfo. The rest of the icons are not displayed anyway

Stan: Meant it's missing a final period. :) Wonder if it's worth showing a warning anymore. Could be…
vladislavbelovAuthorUnsubmitted
Done
Inline Actions

Ah, thanks.
I think yes, because it still can overflow.

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
Wildfire Games · Phabricator