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.

Sign up for GitHub

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

GuiTraceRay from theater ray intersection instead of camera position. #1782

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

GuiTraceRay from theater ray intersection instead of camera position. #1782

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
2a0a27e
GuiTraceRay from theater ray intersection instead of camera position.
saurtron Nov 25, 2024
fab8808
Fix typo.
saurtron Nov 25, 2024
940bc8c
Fix error in plane equation.
saurtron Nov 25, 2024
76e69d4
Keep track of max unit altitude inside UnitHandler.
saurtron Nov 25, 2024
146709b
Make new camera methods const and improve variable name.
saurtron Nov 25, 2024
9d167ce
use float3::dot
saurtron Nov 25, 2024
d89a16d
Change NULL to nullptr at GuiTraceRay calls.
saurtron Nov 25, 2024
5773b9b
Change function name to TracePointToMaxAltitude and ensure proper
saurtron Nov 25, 2024
9850d92
Init unit max altitude to readMap->GetCurrMaxHeight and change how unit
saurtron Nov 27, 2024
cc626ec
Also keep maxaltitude for features.
saurtron Nov 27, 2024
8b8c81e
Fix function call.
saurtron Nov 27, 2024
01c3ef0
The frame number to check needs to be 2^x-1.
saurtron Nov 27, 2024
b15e68a
Improve theater ray intersection by using std::optional and removing
saurtron Nov 27, 2024
0e190f8
Don't calculate theater intersection if bottom frustum isn't parallel to
saurtron Nov 30, 2024
3af3c51
Use fbl.y/100000.0 instead of stl::epsilon since the frustum points
saurtron Dec 1, 2024
ad030ea
Check camera is straight enough by tracing cam to ftl to theater plane.
saurtron Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 52 additions & 0 deletions rts/Game/Camera.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "System/float3.h"
#include "System/Matrix44f.h"
#include "System/Config/ConfigHandler.h"
#include "Sim/Features/FeatureHandler.h"
#include "Sim/Units/UnitHandler.h"

#include "System/Misc/TracyDefs.h"

Expand Down Expand Up @@ -773,6 +775,56 @@ float3 CCamera::GetMoveVectorFromState(bool fromKeyState) const
return v;
}

std::optional<float3> CCamera::TracePointToMaxAltitude(const float3& point, const float rayLength, const float maxAltitude) const
{
// ray from camera position to point, intersecting with the maxAltitude horizontal plane.
const float3 dir = (point-pos).Normalize();
const float dist = CGround::LinePlaneCol(pos, dir, rayLength, maxAltitude);
if (dist > 0.0) {
return pos + dir*dist;
}
return std::nullopt;
}

float3 CCamera::NearTheaterIntersection(const float3& dir, const float rayLength) const
{
// intersect the frustum with max altitude to get the optimal ray start.

// max unit and feature altitudes are always at least map MaxHeight.
const float maxAltitude = std::max <float> (unitHandler.MaxUnitAltitude(), featureHandler.MaxFeatureAltitude());
if (pos.y < maxAltitude)
return pos;

const auto fbl = GetFrustumVert(CCamera::FRUSTUM_POINT_FBL);
const auto fbr = GetFrustumVert(CCamera::FRUSTUM_POINT_FBR);
const auto ftl = GetFrustumVert(CCamera::FRUSTUM_POINT_FTL);

// check the bottom frustum is parallel to ground and lower than the top frustum
if ((std::abs(fbl.y-fbr.y) > std::numeric_limits<float>::epsilon()) || (fbl.y >= ftl.y))
Copy link
Collaborator

Choose a reason for hiding this comment

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

epscmp exists, perhaps use that? Maybe std::numerlic_limits<T>::epsilon() could even become the default for its 3rd arg:

spring/rts/System/SpringMath.h

Line 141 in a2dd682

template<class T> inline bool epscmp(const T a, const T b, const T eps) {

return pos;

const auto fv1 = TracePointToMaxAltitude(fbl, rayLength, maxAltitude);
const auto fv2 = TracePointToMaxAltitude(fbr, rayLength, maxAltitude);
if (!fv1 || !fv2)
return pos;

float3 midFv = (fv1.value()+fv2.value())/2.0;
midFv.y = pos.y;

// vertical plane from frustum intersection to max height
const float3 p = fv1.value();
const float3 norm = midFv-pos;
const auto d = -norm.dot(p);
const float4 nearTheaterPlane = float4(norm.x, norm.y, norm.z, d);

// intersection
float3 intersection;
const bool res = RayAndPlaneIntersection(pos, pos+dir*rayLength, nearTheaterPlane, false, intersection);
if (res)
return intersection;
return pos;
}

// http://www.lighthouse3d.com/tutorials/view-frustum-culling/geometric-approach-testing-points-and-spheres/
bool CCamera::Frustum::IntersectSphere(float3 p, float radius, uint8_t testMask) const
{
Expand Down
3 changes: 3 additions & 0 deletions rts/Game/Camera.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,9 @@ class CCamera {
return (forward.dot(objPos - pos));
}

std::optional<float3> TracePointToMaxAltitude(const float3& point, const float rayLength, const float maxAltitude) const;
float3 NearTheaterIntersection(const float3& dir, const float rayLength) const;

/*
float ProjectedDistanceShadow(const float3& objPos, const float3& sunDir) const {
// FIXME: fix it, cap it for shallow shadows?
Expand Down
39 changes: 26 additions & 13 deletions rts/Game/UI/GuiHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1155,8 +1155,9 @@ bool CGuiHandler::TryTarget(const SCommandDescription& cmdDesc) const
const CFeature* targetFeature = nullptr;

const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float dist = TraceRay::GuiTraceRay(camera->GetPos(), mouse->dir, viewRange, NULL, targetUnit, targetFeature, true);
const float3 groundPos = camera->GetPos() + mouse->dir * dist;
const float3 rayOrigin = camera->NearTheaterIntersection(mouse->dir, viewRange);
const float dist = TraceRay::GuiTraceRay(rayOrigin, mouse->dir, viewRange, nullptr, targetUnit, targetFeature, true);
const float3 groundPos = rayOrigin + mouse->dir * dist;

if (dist <= 0.0f)
return false;
Expand Down Expand Up @@ -1685,8 +1686,11 @@ int CGuiHandler::GetDefaultCommand(int x, int y, const float3& cameraPos, const
unit = minimap->GetSelectUnit(minimap->GetMapPosition(x, y));
} else {
const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float dist = TraceRay::GuiTraceRay(cameraPos, mouseDir, viewRange, nullptr, unit, feature, true);
const float3 hit = cameraPos + mouseDir * dist;

const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, viewRange);
const float dist = TraceRay::GuiTraceRay(rayOrigin, mouseDir, viewRange, nullptr, unit, feature, true);

const float3 hit = rayOrigin + mouseDir * dist;

// make sure the ray hit in the map
if (unit == nullptr && feature == nullptr && !hit.IsInBounds())
Expand Down Expand Up @@ -2265,7 +2269,9 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
const CUnit* unit = nullptr;
const CFeature* feature = nullptr;

TraceRay::GuiTraceRay(cameraPos, mouseDir, camera->GetFarPlaneDist() * 1.4f, nullptr, unit, feature, true);
const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, viewRange);
TraceRay::GuiTraceRay(rayOrigin, mouseDir, viewRange, nullptr, unit, feature, true);

if (unit == nullptr)
return defaultRet;
Expand All @@ -2282,7 +2288,8 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
const CFeature* feature = nullptr;

const float traceDist = camera->GetFarPlaneDist() * 1.4f;
const float isectDist = TraceRay::GuiTraceRay(cameraPos, mouseDir, traceDist, nullptr, unit, feature, true);
const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, traceDist);
const float isectDist = TraceRay::GuiTraceRay(rayOrigin, mouseDir, traceDist, nullptr, unit, feature, true);

if (isectDist > (traceDist - 300.0f))
return defaultRet;
Expand All @@ -2292,7 +2299,7 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
c.PushParam(unit->id);
} else {
// clicked in map
c.PushPos(cameraPos + (mouseDir * isectDist));
c.PushPos(rayOrigin + (mouseDir * isectDist));
}
return CheckCommand(c);
}
Expand Down Expand Up @@ -2345,7 +2352,9 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
if (mouse->buttons[button].movement <= mouse->dragCircleCommandThreshold) {
const CUnit* unit = nullptr;
const CFeature* feature = nullptr;
const float dist2 = TraceRay::GuiTraceRay(cameraPos, mouseDir, camera->GetFarPlaneDist() * 1.4f, NULL, unit, feature, true);
const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, viewRange);
const float dist2 = TraceRay::GuiTraceRay(rayOrigin, mouseDir, viewRange, nullptr, unit, feature, true);

if (dist2 > (camera->GetFarPlaneDist() * 1.4f - 300) && (commands[tempInCommand].type != CMDTYPE_ICON_UNIT_FEATURE_OR_AREA))
return defaultRet;
Expand All @@ -2362,7 +2371,7 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
if (explicitCommand < 0 || !ZeroRadiusAllowed(c))
return defaultRet;

c.PushPos(cameraPos + (mouseDir * dist2));
c.PushPos(rayOrigin + (mouseDir * dist2));
c.PushParam(0); // zero radius

if (c.GetID() == CMD_UNLOAD_UNITS)
Expand Down Expand Up @@ -2408,7 +2417,8 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
const CFeature* feature = nullptr;

const float traceDist = camera->GetFarPlaneDist() * 1.4f;
const float outerDist = TraceRay::GuiTraceRay(cameraPos, mouseDir, traceDist, nullptr, unit, feature, true);
const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, traceDist);
const float outerDist = TraceRay::GuiTraceRay(rayOrigin, mouseDir, traceDist, nullptr, unit, feature, true);

if (outerDist > (traceDist - 300.0f))
return defaultRet;
Expand All @@ -2421,7 +2431,7 @@ Command CGuiHandler::GetCommand(int mouseX, int mouseY, int buttonHint, bool pre
if (explicitCommand < 0)
return defaultRet;

c.PushPos(cameraPos + (mouseDir * outerDist));
c.PushPos(rayOrigin + (mouseDir * outerDist));
}
} else {
// create rectangular area-command
Expand Down Expand Up @@ -2503,7 +2513,9 @@ size_t CGuiHandler::GetBuildPositions(const BuildInfo& startInfo, const BuildInf
const CUnit* unit = nullptr;
const CFeature* feature = nullptr;

TraceRay::GuiTraceRay(cameraPos, mouseDir, camera->GetFarPlaneDist() * 1.4f, nullptr, unit, feature, startInfo.def->floatOnWater);
const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float3 rayOrigin = camera->NearTheaterIntersection(mouseDir, viewRange);
TraceRay::GuiTraceRay(rayOrigin, mouseDir, viewRange, nullptr, unit, feature, startInfo.def->floatOnWater);

if (unit != nullptr) {
other.def = unit->unitDef;
Expand Down Expand Up @@ -3710,7 +3722,8 @@ void CGuiHandler::DrawMapStuff(bool onMiniMap)
unit = minimap->GetSelectUnit(tracePos);
} else {
// ignore the returned distance, we don't care about it here
TraceRay::GuiTraceRay(tracePos, traceDir, maxTraceDist, nullptr, unit, feature, false);
const float3 rayOrigin = camera->NearTheaterIntersection(traceDir, maxTraceDist);
TraceRay::GuiTraceRay(rayOrigin, traceDir, maxTraceDist, nullptr, unit, feature, false);
}

if (unit != nullptr && (gu->spectatingFullView || unit->IsInLosForAllyTeam(gu->myAllyTeam))) {
Expand Down
9 changes: 6 additions & 3 deletions rts/Game/UI/MouseHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,9 @@ void CMouseHandler::MouseRelease(int x, int y, int button)
const CUnit* unit = nullptr;
const CFeature* feature = nullptr;

TraceRay::GuiTraceRay(camera->GetPos(), dir, camera->GetFarPlaneDist() * 1.4f, nullptr, unit, feature, false);
const float viewRange = camera->GetFarPlaneDist() * 1.4f;
const float3 rayOrigin = camera->NearTheaterIntersection(dir, viewRange);
TraceRay::GuiTraceRay(rayOrigin, dir, viewRange, nullptr, unit, feature, false);
lastClicked = unit;

const bool selectType = (bp.lastRelease >= (gu->gameTime - doubleClickTime) && unit == _lastClicked);
Expand Down Expand Up @@ -701,8 +703,9 @@ std::string CMouseHandler::GetCurrentTooltip() const
const CUnit* unit = nullptr;
const CFeature* feature = nullptr;

const float3 rayOrigin = camera->NearTheaterIntersection(dir, range);
{
dist = TraceRay::GuiTraceRay(camera->GetPos(), dir, range, nullptr, unit, feature, true, false, true);
dist = TraceRay::GuiTraceRay(rayOrigin, dir, range, nullptr, unit, feature, true, false, true);

if (unit != nullptr) return CTooltipConsole::MakeUnitString(unit);
if (feature != nullptr) return CTooltipConsole::MakeFeatureString(feature);
Expand All @@ -714,7 +717,7 @@ std::string CMouseHandler::GetCurrentTooltip() const
return selTip;

if (dist <= range)
return CTooltipConsole::MakeGroundString(camera->GetPos() + (dir * dist));
return CTooltipConsole::MakeGroundString(rayOrigin + (dir * dist));

return "";
}
Expand Down
2 changes: 2 additions & 0 deletions rts/Lua/LuaSyncedCtrl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3261,6 +3261,7 @@ int LuaSyncedCtrl::SetUnitMidAndAimPos(lua_State* L)

if (updateQuads) {
quadField.MovedUnit(unit);
unitHandler.MovedUnit(unit);
}

lua_pushboolean(L, true);
Expand Down Expand Up @@ -3296,6 +3297,7 @@ int LuaSyncedCtrl::SetUnitRadiusAndHeight(lua_State* L)

if (updateQuads) {
quadField.MovedUnit(unit);
unitHandler.MovedUnit(unit);
}

lua_pushboolean(L, true);
Expand Down
11 changes: 8 additions & 3 deletions rts/Lua/LuaUnsyncedRead.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2952,10 +2952,15 @@ int LuaUnsyncedRead::TraceScreenRay(lua_State* L)
const float3 pxlDir = camera->CalcPixelDir(wx, wy);

// trace for player's allyteam
const float traceDist = TraceRay::GuiTraceRay(camPos, pxlDir, rawRange, nullptr, unit, feature, true, onlyCoords, ignoreWater);
float3 rayOrigin;
if (onlyCoords)
rayOrigin = camPos;
else
rayOrigin = camera->NearTheaterIntersection(pxlDir, rawRange);
const float traceDist = TraceRay::GuiTraceRay(rayOrigin, pxlDir, rawRange, nullptr, unit, feature, true, onlyCoords, ignoreWater);
const float planeDist = CGround::LinePlaneCol(camPos, pxlDir, rawRange, luaL_optnumber(L, newArgIdx, 0.0f));

const float3 tracePos = camPos + (pxlDir * traceDist);
const float3 tracePos = rayOrigin + (pxlDir * traceDist);
const float3 planePos = camPos + (pxlDir * planeDist); // backup (for includeSky and onlyCoords)

if ((traceDist < 0.0f || traceDist > badRange) && unit == nullptr && feature == nullptr) {
Expand Down Expand Up @@ -4890,4 +4895,4 @@ int LuaUnsyncedRead::SolveNURBSCurve(lua_State* L)
lua_rawseti(L, -2, ++i);
}
return 1;
}
}
5 changes: 3 additions & 2 deletions rts/Rendering/UniformConstants.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,10 @@ void UniformConstants::UpdateParamsImpl(UniformParamsBuffer* updateBuffer)
const float3 pxlDir = camPlayer->CalcPixelDir(wx, wy);

// trace for player's allyteam
const float traceDist = TraceRay::GuiTraceRay(camPos, pxlDir, rawRange, nullptr, unit, feature, true, false, true);
const float3 rayOrigin = camPlayer->NearTheaterIntersection(pxlDir, rawRange);
const float traceDist = TraceRay::GuiTraceRay(rayOrigin, pxlDir, rawRange, nullptr, unit, feature, true, false, true);

const float3 tracePos = camPos + (pxlDir * traceDist);
const float3 tracePos = rayOrigin + (pxlDir * traceDist);

if (unit)
updateBuffer->mouseWorldPos = float4{ unit->drawPos, 1.0f };
Expand Down
2 changes: 2 additions & 0 deletions rts/Sim/Features/Feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -481,6 +481,7 @@ void CFeature::ForcedMove(const float3& newPos)
UpdateTransformAndPhysState();

eventHandler.FeatureMoved(this, oldPos);
featureHandler.MovedFeature(this);

// insert into managers
quadField.AddFeature(this);
Expand Down Expand Up @@ -596,6 +597,7 @@ bool CFeature::UpdatePosition()
// use an exact comparison for the y-component (gravity is small)
if (!pos.equals(oldPos, float3(float3::cmp_eps(), 0.0f, float3::cmp_eps()))) {
eventHandler.FeatureMoved(this, oldPos);
featureHandler.MovedFeature(this);
return true;
}

Expand Down
26 changes: 26 additions & 0 deletions rts/Sim/Features/FeatureHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#include "Sim/Ecs/Registry.h"
#include "Sim/Misc/QuadField.h"
#include "Sim/Units/CommandAI/BuilderCAI.h"
#include "Sim/Units/UnitHandler.h"
#include "System/creg/STL_Set.h"
#include "System/EventHandler.h"
#include "System/TimeProfiler.h"
Expand Down Expand Up @@ -39,6 +40,7 @@ void CFeatureHandler::Init() {
features.resize(MAX_FEATURES, nullptr);
activeFeatureIDs.reserve(MAX_FEATURES); // internal table size must be constant
featureMemPool.reserve(128);
maxFeatureAltitude = readMap->GetCurrMaxHeight();

idPool.Clear();
idPool.Expand(0, MAX_FEATURES);
Expand All @@ -58,6 +60,7 @@ void CFeatureHandler::Kill() {
deletedFeatureIDs.clear();
features.clear();
updateFeatures.clear();
maxFeatureAltitude = std::numeric_limits<float>::lowest();
}


Expand Down Expand Up @@ -141,6 +144,7 @@ bool CFeatureHandler::AddFeature(CFeature* feature)

InsertActiveFeature(feature);
SetFeatureUpdateable(feature);
MovedFeature(feature);
return true;
}

Expand Down Expand Up @@ -186,6 +190,19 @@ CFeature* CFeatureHandler::CreateWreckage(const FeatureLoadParams& cparams)
}


void CFeatureHandler::RecalculateMaxAltitude()
{
if (maxFeatureAltitude < std::max(readMap->GetCurrMaxHeight(), unitHandler.MaxUnitAltitude()))
return;

maxFeatureAltitude = readMap->GetCurrMaxHeight();

for (const int featureID: activeFeatureIDs) {
CFeature* f = features[featureID];
MovedFeature(f);
}
}


void CFeatureHandler::Update()
{
Expand All @@ -203,6 +220,9 @@ void CFeatureHandler::Update()

updateFeatures.erase(iter, updateFeatures.end());
}
if ((gs->frameNum & 63) == 0) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I set it to 62 because 2 secs felt right

How about

Suggested change
if ((gs->frameNum & 63) == 0) {
/* Value arbitrary, represents a perf tradeoff:
* larger values decrease per-feature overhead over time for the usual state where features don't move vertically,
* smaller values decrease traceray overhead if features often enter and leave the stratosphere. */
static constexpr auto UPDATE_RATE = 2 * GAME_SPEED;
if (gs->frameNum % UPDATE_RATE == 0) {

Copy link
Collaborator Author

@saurtron saurtron Nov 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Shouldn't we also change the check above this for destroyed features for consistency too? Otherwise it feels a bit ad-hoc. I did it like this for internal consistency in the class tbh.

Also one concern here is everyone doing checks on frameNum % (n * GAME_SPEED), then all n multipliers of game speed would see perf hit with everyone queueing to do extra work there. So maybe want to add an offset here.

Right now all modules choose "slots" quite randomly resulting in random frames with increased load I guess.

Ideally maybe everyone should query for free/next update slots with some given period stepping so work is distributed among those, probably neither % or & standalone are really good in the end.

Maybe smth like:

// example for doing specific slot in 32 (2⁵ frame groups).
static constexpr int PERIOD_STEPPING = 5;
static constexpr int UPDATE_MASK = std::pow(2, PERIOD_STEPPING)-1;
auto slot = engine->getNextSlot(PERIOD_STEPPING);

 if ((frameNum+slot) & UPDATE_MASK))
    // do work..

I think it could even be improved to sync between different periods. Lua could also use the mechanism, the only issue would be methods who want to do it right now and then with a specific period, but for those probably best would be to actually do it right now somehow, and then conform to the slot assigned by the engine.

Right now here, I think the consistency is important so the function uses the same method for both checks. Later maybe a method like above could be used to properly distribute load among processes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we also change the check above this for destroyed features for consistency too? Otherwise it feels a bit ad-hoc. I did it like this for internal consistency in the class tbh.

Eventually yes. There are many x & 31 left all over engine that should mostly become x % GAME_SPEED at some point, but some care needs to be taken for each to make sure there isn't a regression. In particular I am unsure if the one above is safe to change this way because running at a slightly slower rate than unit CommandAI slow updates may be on purpose to make sure CAI has an opportunity to process things.

Right now here, I think the consistency is important so the function uses the same method for both checks.

Sure. I don't mind either way.

everyone doing checks on frameNum % (n * GAME_SPEED), then all n multipliers of game speed would see perf hit with everyone queueing to do extra work there. So maybe want to add an offset here. Right now all modules choose "slots" quite randomly resulting in random frames with increased load I guess.

Sure. Most of this sort of periodic work is reasonably light though, I'd expect everything just has an offset of 0 but there aren't really spikes every second. Also there may be hidden assumptions so again it would take some care to make sure not to break things (e.g. handler A's periodic update must run right after handler B's and it's not made explicit anywhere, but it sort of happens to work because they just currently have the same offset and period - I remember something like this in resource income stat collection but there may be others).

Lua could also use the mechanism, the only issue would be methods who want to do it right now and then with a specific period

Lua is a monolith from engine PoV so it is up to Lua to arrange work for individual wupgets, e.g. they could sign up dynamically to the wupgetHandler or use Script.DelayByFrames.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

care needs to be taken for each to make sure there isn't a regression.

Indeed, any such rework would have to be done with extreme care to understand everything going on.

Also there may be hidden assumptions so again it would take some care to make sure not to break things (e.g. handler A's periodic update must run right after handler B's and it's not made explicit anywhere, but it sort of happens to work because they just currently have the same offset and period

Current situation is fragile tho, because there might be such a situation happening and might be hard to notice, and any day someone may change some of the periods but then break any such assumptions, like if we decide to change both periods at featureHandler in this PR.

RecalculateMaxAltitude();
}
}


Expand Down Expand Up @@ -287,3 +307,9 @@ void CFeatureHandler::TerrainChanged(int x1, int y1, int x2, int y2)
}
}

void CFeatureHandler::MovedFeature(const CFeature* feature)
{
const CollisionVolume& cv = feature->selectionVolume;
const float top = cv.GetWorldSpacePos(feature).y + cv.GetBoundingRadius();
maxFeatureAltitude = std::max(top, maxFeatureAltitude);
}
9 changes: 9 additions & 0 deletions rts/Sim/Features/FeatureHandler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,12 +59,15 @@ class CFeatureHandler : public spring::noncopyable
bool TryFreeFeatureID(int id);
bool AddFeature(CFeature* feature);
void DeleteFeature(CFeature* feature);
void MovedFeature(const CFeature* feature);

void LoadFeaturesFromMap();

void SetFeatureUpdateable(CFeature* feature);
void TerrainChanged(int x1, int y1, int x2, int y2);

float MaxFeatureAltitude() const { return maxFeatureAltitude; }

const spring::unordered_set<int>& GetActiveFeatureIDs() const { return activeFeatureIDs; }

private:
Expand All @@ -88,6 +91,12 @@ class CFeatureHandler : public spring::noncopyable
std::vector<int> deletedFeatureIDs;
std::vector<CFeature*> features;
std::vector<CFeature*> updateFeatures;

///< highest altitude of any feature added so far
///< (ray tracing uses this in some cases)
float maxFeatureAltitude = 0.0f;

void RecalculateMaxAltitude();
};

extern CFeatureHandler featureHandler;
Expand Down
Loading