Add celltrigger support to ProximityCapturable #21123
Conversation
| // from which a border should be drawn. The index of the end corner will be (cornerIndex + 1) % 4. | ||
| static readonly Dictionary<CVec, int> Offset2CornerIndex = new() | ||
| { | ||
| { new CVec(0, -1), 0 }, |
There was a problem hiding this comment.
consider making the index an enum i.e. north, east, south, west. even though it is private.
There was a problem hiding this comment.
Done (kinda). The index represents corners of a cell, not cardinal directions.
| var corners = map.Grid.Ramps[ramp].Corners; | ||
| var pos = map.CenterOfCell(c) - new WVec(0, 0, map.Grid.Ramps[ramp].CenterHeightOffset); | ||
|
|
||
| foreach (var o in Offset2CornerIndex) |
There was a problem hiding this comment.
it would be no option to calculate the border once during construction of a bordered region and be able to render that as a multi line?
it would be so much cheaper than iterating over all cells in the region and then for each cell checking all four neigbouring cells.
or would the region change? i would imagine celltriggers typically remain at one position of the map? i read below the region is already relative to the offset of the actor.
perhaps have a constant bordered region for actors that cannot move. or a relative bordered region.
There was a problem hiding this comment.
it would be no option to calculate the border once during construction of a bordered region and be able to render that as a multi line?
I gave that a try, but there's a bunch of problems with that. 1) the shroud check requires that the set of drawn lines gets determined every time they get drawn, 2) you'd have to sort the lines so that the ending vertex of a preceding line matches the starting vertex of the following line. I haven't found a nice way to do this, and it probably wouldn't be any cheaper.
Also, in the end the color renderer just draws a bunch of quads individually anyway, so the gain of drawing a multi-line is probably negligible (I guess, no idea really what's costly in terms of drawing).
or would the region change? i would imagine celltriggers typically remain at one position of the map?
Typically not. I suppose moving actors would normally use a ProximityTrigger. When the actor gets removed and added back to the world, the region could theoretically change, though.
| if (region.Contains(c + o.Key)) | ||
| continue; | ||
|
|
||
| var start = wr.Viewport.WorldToViewPx(wr.Screen3DPosition(pos + corners[o.Value])); |
There was a problem hiding this comment.
check contrastWidth and width prior. to avoid calculating the positions.
the whole render is not needed with both widths are zero?
| using OpenRA.Primitives; | ||
| using OpenRA.Traits; | ||
|
|
||
| namespace OpenRA.Mods.Common.Traits | ||
| { | ||
| public enum ProximityCapturableType { Range, Region } |
There was a problem hiding this comment.
optional. code would be slightly cleaner if you'd split both types up in two traits. i.e. add RangeProximityCapturable.
as the fields per info are different.
There was a problem hiding this comment.
no option for a Region + Range? i guess ActorMap would have to support it too.
There was a problem hiding this comment.
optional. code would be slightly cleaner if you'd split both types up in two traits. i.e. add RangeProximityCapturable.
Done.
no option for a Region + Range? i guess ActorMap would have to support it too.
No. That would require two different triggers.
obrakmann
force-pushed
the
more-prox-capturable-improvements2
branch
from
515352b
to
88c4557
Compare
There was a problem hiding this comment.
untested.
would like to propose to get some performance metrics on the impact of drawing the borders but also the impact on updating the actor map at each actor movement. i.e. what if a modder add the trait to many/any actor, at what point does it no longer scale.
This PR doesn't do that. The call to |
i know. but this would encourage the use(?). |
|
Still no :). And I don't see how adding one trait would encourage the use of the previously already existing trait. |
| [Desc("Allowed ProximityCaptor actors to capture this actor.")] | ||
| public readonly BitSet<CaptureType> CaptorTypes = new("Player", "Vehicle", "Tank", "Infantry"); | ||
|
|
||
| [Desc("If set, the capturing process stops immediately after another player comes into Range.")] |
There was a problem hiding this comment.
Why the capitialized Range?
| { | ||
| var pci = rules.Actors[SystemActors.Player].TraitInfoOrDefault<ProximityCaptorInfo>(); | ||
| if (pci == null) | ||
| throw new YamlException("ProximityCapturableBase requires the `Player` actor to have the ProximityCaptor trait."); |
| [Desc("Actor can be captured by units in a specified proximity.")] | ||
| public abstract class ProximityCapturableBaseInfo : TraitInfo, IRulesetLoaded | ||
| { | ||
| [Desc("Allowed ProximityCaptor actors to capture this actor.")] |
| public readonly bool Sticky = false; | ||
|
|
||
| [Desc("If set, the actor can only be captured via this logic once.", | ||
| "This option implies the `Sticky` behaviour as well.")] |
| if (!self.IsInWorld || !Info.DrawDecoration) | ||
| yield break; | ||
|
|
||
| yield return GetRenderable(self, wr); |
There was a problem hiding this comment.
if (!self.IsInWorld || !Info.DrawDecoration)
return Enumerable.Empty<IRenderable>();
return new[] { GetRenderable(self, wr); }Minor, but avoids allocating an enumerable if we don't want to draw decorations.
| { | ||
| [Desc("Set of cell offsets (relative to the actor's Location) the ProximityCaptor needs to be in to initiate the capture. ", | ||
| "A 'Region' ActorInit can be used to override this value per actor. If either is empty or non-existent, ", | ||
| "the immediately neighboring cells of the actor will be used.")] |
There was a problem hiding this comment.
Done, except for the ActorInit. It would say "RegionInit" and I suppose that would confuse modders (ie. they'd try adding "RegionInit" to the yaml instead of just "Region".
I was thinking about |
obrakmann
force-pushed
the
more-prox-capturable-improvements2
branch
from
88c4557
to
58173aa
Compare
obrakmann
force-pushed
the
more-prox-capturable-improvements2
branch
from
58173aa
to
a73dfda
Compare
Again, this is adding |
obrakmann
force-pushed
the
more-prox-capturable-improvements2
branch
from
a73dfda
to
3889e4c
Compare
|
Removed testcase commit |
This pull request addresses a long-standing TODO comment in
ProximityCapturableabout eventually adding celltrigger support.