GuiTraceRay from theater ray intersection instead of camera position. #1782
Conversation
* Theater is the vertical plane defined by the intersection of camera to far bottom frustum corners with the map maxheight+maxunitaltitude. * Calculate the theater ray intersection as the intersection of the theater plane with the ray.
|
Units can be arbitrarily high due to impulse and Lua so there is no such thing as maximum altitude. |
Well, in practice atm this is not the case so I think it's worth it to consider. In bar in a typical map with some high areas the max map altitude can be 500 or 1000, units max altitude around 500. Camera can be at 4000 or 5000+. Anyways, even in the case there's no absolute maximum it can be considered to maintain the current highest altitude as an unsynced value, actually I thought the engine might already be maintaining something like this itself but couldn't check properly yet. |
|
ZK has orbital flingers so the "in practice" isn't even true, but if it was I still wouldn't want to sacrifice capabilities / behaviour correctness in name of perf, as a matter of principle. Keeping some sort of max visible object height cached (such that the behaviour is always correct) sounds fine. |
|
Added keeping track of unit altitude.
Of course I also wouldn't do it, the initial value was just a placeholder while discussing, was just proposing either we keep track of absolute altitude (as implemented atm) or have some kind of limit set that would be impossible to breach unless the game sets it higher in some way, or maybe the engine had something already, but looking at it doesn't seem to be the case. Actually if you think about it, maybe the engine does have some higher limit by itself, due to different variables (like unitdef), we just don't know what it is. Of course lua can set it higher, but we can track the limit when lua changes something. Anyways for now I'm updating the limit pretty much everywhere quadField is getting updated, plus also resetting it on UnitHandler UpdateUnits as that seemed reasonable. I think that should be ok. The thing i'm not totally sure atm is I'm keeping the limit with |
Co-authored-by: sprunk <[email protected]>
It probably should use the top of the selection volume which probably means some nonsense like Features are also subject to the trace-ray so should be included in the bounds calculation. Both units and features are derived from the CSolidObject class which is the one that has selectionVolume as a member so the handling would be the same. |
behaviour when no intersection is found.
top is calculated.
Added this, although I think it'd be good to find a lighter method. Note we don't need exact results but just to be higher than the exact results. Anyways it's not extremely expensive, but we will be looping all units all the time to keep this updated (otherwise there's no way to reset the altitude and it would just keep going up), plus running it on UnitMove.
Added feature support too. I added it inside featureHandler in a similar way to how unitHandler is doing it. I'm doing the altitude reset less often though, as I don't expect this to go up a lot also there is no loop already on features so I had to add one. The good thing is we can check earlier and if the feature max altitude is lower than ground or unit altitude we don't need to loop it. |
superfluous checking of map MaxHeight.
|
If #1774 is to be merged then everything that updates the respective max should have a visibility check, otherwise the new |
Yes I was thinking to make that cheat mode only. |
|
Also works. |
| @@ -203,6 +220,9 @@ void CFeatureHandler::Update() | |||
|
|
|||
| updateFeatures.erase(iter, updateFeatures.end()); | |||
| } | |||
| if ((gs->frameNum & 63) == 0) { | |||
There was a problem hiding this comment.
I set it to 62 because 2 secs felt right
How about
| 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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
diagram to document the theater idea:
ray origin is the same as camera position, note how it's much before any possible intersection with units |
ground or higher than top frustum.
rts/Game/Camera.cpp
Outdated
| 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)) |
There was a problem hiding this comment.
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
can have error much bigger than epsilon even when the camera is parallel to groung.
|
When analyzing the PR with @lhog I noticed the terrible overscanning is only happening when zoom fov is being used (not normal by default, at least in BAR). In normal situation it's still overscanning when not looking more or less down, although it's not that bad. Also fixed some error conditions when determininig the closest point. Now I believe it works good. Still, I think this PR would benefit from some profiling before thinking about merge, since under "normal" conditions I believe it's not overscanning too much for most users (just for the ones who use more of a Supreme commander style camera). Also likely the Camera::NearTheaterIntersection method could have a cheaper test to bail out early, but still the main performance cost in this PR is keeping all altitudes updated. updated image with normal camera overtracing representationI posted the updated image at PR description for reference, but posting here as well: (check the total longitude of the ray, not the width, width is a bit different because angle and position of ray is a bit different in both images, so ray width is affecting differently) at the top images you can see the blue square (camera pos), that's where we normally trace from. note: previously it was proposed here to use near frustum to mouse ray intersection as starting point, but that seems to be (almost) the same as camera position, needs to be investigated further.
|
Work done
Explanation
Background
Implementation
Remarks
Before and after
Diagram