Page MenuHomeWildfire Games

Allow selectable component to overwrite shape of displayed selection from footprint
ClosedPublic

Authored by Angen on Jun 28 2020, 3:37 PM.

Details

Summary

Based on D2496, D2721, D2640 making selection look good is strictly bound with gameplay balancing because width and height for drawing selection is taken from footprint and that one is used to detect arrow hits.

This is removing strict binding of selection to footprint by adding optional choice similar in footprint. If present, that one will be used, else it will fallback to footprint size. (This fallback is kept because in most cases at least now in svn it is enough to use footprint sizes and to not overwhelm moders with this change.)

Also important for this comment https://wildfiregames.com/forum/index.php?/topic/28216-selection-shapes/&do=findComment&comment=396973. This would allow fancier selection shapes.

Test Plan

Confirm tweaked selections are applied.
Confirm when element in selection is not present, default selection from footprint is displayed.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Angen created this revision.Jun 28 2020, 3:37 PM
Owners added subscribers: Restricted Owners Package, Restricted Owners Package.Jun 28 2020, 3:37 PM

Build failure - The Moirai have given mortals hearts that can endure.

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2548/display/redirect

The idea is nice, it would allow heroes to have the same footprints as their citizen counterparts while keeping their star overlay textures square.
However, currently the footprint is the area an entity occupies and the <Selectable> node is its visualization. By detaching the latter from the former, there is no way in game for players to know what the actual footprint is, and the <Selectable> becomes effectively meaningless. Maybe @elexis, @fatherbushido, and others have an opinion on this?
Have you considered alternatives? E.g. use the <VisualActor/Actor> for calculating the hit area? Or keep <Footprint> and <Selectable> as is, but introduce a separate <HitBox> node? That would also be quite useful for structures where the actual building occupies only half of the footprint (e.g. maur_elephant_stables.xml, rome_civil_centre.xml).
Also, shouldn't the circle or square be moved inside or merged with the <Overlay>? And does your implementation work with entities that use an <Outline> instead of a <Texture> (e.g. template_gaia.xml, template_structure.xml)?
Furthermore, what happens if two entities have footprints that don't overlap, but the <Selectable> area does, and you click on the overlapping part? Which entity is selected?

Part of the problem with "fancier" selection shapes is that you tend to have to use a larger footprint size for the fancy texture to stand out and be seen. Now, in other games this isn't a problem, but with 0ad footprint does double duty as the entity's combat hitbox. I found this out when I did my tests for a super fancy selection ring for the temple, to show its faith healing range. Every projectile that fell within the radius of that footprint registered as a hit on the temple. I had to remove the awesome fancy footprint texture as a consequence.

This sounds as something that should be done in the aura file, not in the temple template.

Angen added a comment.Jun 29 2020, 3:31 PM

use the <VisualActor/Actor> for calculating the hit area?

Simulation should be independent of graphic and art so no.

keep <Footprint> and <Selectable> as is, but introduce a separate <HitBox> node

That would be more problematic because it would mean melee units will hit places which ranged not as melee move based on obstructions.

However, currently the footprint is the area an entity occupies and the <Selectable> node is its visualization.

No it is not by default. Binding between selectable and footprint was introduced in rP7479 to better mimic shape of entity. It was just constant circle until then.
Also area that entity occupies is not very correct. Entity has obstruction and that area is occupied by entity and cannot be build on and is collided when moving.
Footprint is just hitbox for ranged attacks and unit spawning to spawn nicely since their obstruction is too small and they would one over another when spawned.

Furthermore, what happens if two entities have footprints that don't overlap, but the <Selectable> area does, and you click on the overlapping part? Which entity is selected?

This is not changing how selection works, just how it is visualised. Selectable component has nothing to do with fact how selection is done.

Also, shouldn't the circle or square be moved inside or merged with the <Overlay>?

Hmm, yes it probably should.

And does your implementation work with entities that use an <Outline> instead of a <Texture>

I did not change how are things rendered. Just allowed to read data which overwrite data from footprint. Combinations among Square/Circle and Otline/Texture works just as if defined only in footprint.
And yes we can even question now if it would not be better to define that shapes in selection for selection purposes, but just to allow to have crazy footprints for selection without affecting gameplay this is enough to do without need to edit every second template. ;).

No one is forced to use this and anyone can stay with shapes defined by footprint.
If it makes confusing to include D2496 because it displays shapes of cavalry and elephants too different from footprints, I ll remove it and it can be used for obvious cases where footprints are bigger than entities and they are making them bigger targets as they should and no vice versa.

wraitii added a subscriber: wraitii.

The trouble, I suppose, is that obstruction is mostly a pathfinder thing and the game needs another form of "hitbox", which is the footprint. I feel like renaming "Footprint" to "Hitbox" might actually be desirable.

I'll give this a deeper look later.

No it is not by default. Binding between selectable and footprint was introduced in rP7479 to better mimic shape of entity. It was just constant circle until then.

rP7479 was committed ten years ago, before the first alpha.

Also area that entity occupies is not very correct. Entity has obstruction and that area is occupied by entity and cannot be build on and is collided when moving.
Footprint is just hitbox for ranged attacks and unit spawning to spawn nicely since their obstruction is too small and they would one over another when spawned.

Units have obstruction, but unlike structures, no obstruction size is defined in unit templates, so how is unit obstruction calculated then? From the footprint?

This is not changing how selection works, just how it is visualised. Selectable component has nothing to do with fact how selection is done.

Are you sure? I just removed the entire <Selectable> node from template_gaia.xml and as a consequence I could neither select nor gather from mines.

No one is forced to use this and anyone can stay with shapes defined by footprint.

True. I like the idea of this patch, I'm just unsure whether this approach is the best option, partly because I don't understand everything the footprint, obstruction, and selectable nodes exactly do.

If it makes confusing to include D2496 because it displays shapes of cavalry and elephants too different from footprints, I ll remove it and it can be used for obvious cases where footprints are bigger than entities and they are making them bigger targets as they should and no vice versa.

Yes, I think it's better to exclude the template changes here and do that in a separate patch.

The trouble, I suppose, is that obstruction is mostly a pathfinder thing and the game needs another form of "hitbox", which is the footprint. I feel like renaming "Footprint" to "Hitbox" might actually be desirable.

If the only thing footprints do is define an hitbox, then perhaps, though even then I think keeping footprint unrenamed is fine.

Angen added a comment.Jun 29 2020, 6:46 PM

By saying how selection is done I meant it does not define if mouse pointer is actually hitting unit so it is selected. It just defines if it can be selected at all.

Obstruction of unit is defined as "Unit". So obstruction sizes are taken from passability class in unitmotion.
<PassabilityClass>large</PassabilityClass>

By saying how selection is done I meant it does not define if mouse pointer is actually hitting unit so it is selected. It just defines if it can be selected at all.

Thanks for the clarification. What determines then the space where clicking would select an entitiy? The footprint?

Obstruction of unit is defined as "Unit". So obstruction sizes are taken from passability class in unitmotion.

Is <Clearance> in simulation/data/pathfinder.xml the unit obstruction size then?

Angen added a comment.Jun 30 2020, 7:59 AM

Thanks for the clarification. What determines then the space where clicking would select an entitiy? The footprint?

Visual component so entity mesh.

Is <Clearance> in simulation/data/pathfinder.xml the unit obstruction size then?

Yes, it is.

Stan added a subscriber: Stan.Jun 30 2020, 9:32 AM

Then why can't you select a building if the template height is set to 0 ?

In D2844#122005, @Stan wrote:i

Then why can't you select a building if the template height is set to 0 ?

For selection I think we're using the bounding box, so height is taken into account. It's still the 'footprint' component, which I am now inching closer and close to calling the hitbox.

Angen added a comment.EditedJun 30 2020, 10:30 AM

Well I did not my homework too good,

to be exact, it depends which entity do you choose.

For most of them it is calculated from Model.
But there is possibility to redefine bounds in VisualActor node, There is SelectionShape which can be set to Footprint.
So footprint is used for selection in case of:
binaries/data/mods/public/simulation/templates/structures/brit_wonder.xml,
binaries/data/mods/public/simulation/templates/template_gaia_flora.xml,
binaries/data/mods/public/simulation/templates/template_structure_resource_field.xml

I did not found any other connection with footprint for doing selection, but there may be more hidden

So footprint is used for selection in case of:

binaries/data/mods/public/simulation/templates/structures/brit_wonder.xml,
binaries/data/mods/public/simulation/templates/template_gaia_flora.xml,
binaries/data/mods/public/simulation/templates/template_structure_resource_field.xml

For the record, Uffington Horse and field because they're flat, flora because for small circular footprints, the <Outline> approximation looks poorer compared to the <Texture> (D1765#74375).

Angen updated this revision to Diff 12640.Jul 12 2020, 12:04 PM

remove template changes
fix if style

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/ICmpSelectable.h
|   1| /*·Copyright·(C)·2018·Wildfire·Games.
|    | [NORMAL] LicenseYearBear:
|    | License should have "2020" year instead of "2018"

source/simulation2/components/ICmpSelectable.h
|  25| class·ICmpSelectable·:·public·IComponent
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classICmpSelectable:' is invalid C code. Use --std or --language to configure the language.

source/simulation2/components/CCmpSelectable.cpp
|  64| »   DEFAULT_COMPONENT_ALLOCATOR(Selectable)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If DEFAULT_COMPONENT_ALLOCATOR is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2620/display/redirect

Stan added inline comments.Jul 12 2020, 1:43 PM
source/simulation2/components/CCmpSelectable.cpp
114 ↗(On Diff #12640)

Is it really meters?

565 ↗(On Diff #12640)

Might be worth considering whether they affect simulation and if they don't use float instead of fixed.

source/simulation2/components/ICmpSelectable.h
28 ↗(On Diff #12640)

Does it make sense to duplicate that struct?

Angen added inline comments.Jul 12 2020, 2:04 PM
source/simulation2/components/ICmpSelectable.h
28 ↗(On Diff #12640)

I need NONE instead just CIRLCE and SQUARE to know if it was set or not

Stan added inline comments.Jul 12 2020, 2:26 PM
source/simulation2/components/ICmpSelectable.h
28 ↗(On Diff #12640)

Can't it be added to the other one?

wraitii requested changes to this revision.Jul 12 2020, 6:25 PM

To set things a little straight:

  • Obstruction is intended to be the pathfinding hitbox.
  • Footprint is intended to be the actual hitbox.
  • Selectable is responsible for rendering the overlay
  • VisualActor may use the footprint to compute the visibility bounding box.

My suggestions:

  • Move the Enum to CCmpSelectable (not needed anywhere else) and use 'FOOTPRINT/CIRCLE/SQUARE' where FOOTPRINT corresponds to your NONE. It's more explicit then.
  • Clarify the top-level comment in CCmpFootprint, saying that it may be used to determine the selectable outline/visual bounding box.
  • Use w and h instead of 0 and 1 as suffixes (more consistent with other code).
This revision now requires changes to proceed.Jul 12 2020, 6:25 PM
Angen updated this revision to Diff 12700.Jul 15 2020, 8:11 PM

wraitii's comments.
Edit scheme.
Clarify a:help in footprint.
Use Shape encapsulation for better a:help explanation.

<Overlay>
  <Shape>  <- optional
     <Square .../>
  </Shape>
  <Texture>
  ....
  </Texture>
</Overlay>
Angen edited the summary of this revision. (Show Details)Jul 15 2020, 8:11 PM

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpSelectable.cpp
|  71| »   DEFAULT_COMPONENT_ALLOCATOR(Selectable)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If DEFAULT_COMPONENT_ALLOCATOR is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2656/display/redirect

wraitii requested changes to this revision.Jul 16 2020, 9:54 AM

Seems OK, but there are a few issues:

  • See inline for a straight bug
  • I've been unable to use Shape with Outline (template validation issue?)
  • This is minor, but you changed to m_Size_w/h instead of m_SizeW/H, and also I failed to notice that you're using 'width/depth' which seems to make sense, so it would be W/D - I find that less obvious, so maybe just explicitly use Width and Depth. Doesn't have to be changed imo, but since you have to make changes anyways.

It's also not entirely obvious that 'Square' won't result in a square outline -> if the overlay is a round texture, it makes an ellipse. It might actually be better to just have 'width' and 'depth' elements, instead of shapes.

source/simulation2/components/CCmpSelectable.cpp
184 ↗(On Diff #12700)

You're missing a GetChild("Overlay") here.

200 ↗(On Diff #12700)

Probably worth at least a LOGWARNING, possibly an ENSURE.

This revision now requires changes to proceed.Jul 16 2020, 9:54 AM
Angen added a comment.Jul 17 2020, 8:12 AM

I've been unable to use Shape with Outline (template validation issue?)

I am not sure what is wrong with it. It worked in previous diff and all I did was to surround choice with Shape and moved it inside overlay.
It looks like it does not look at the schema ? or schema is somewhat wrong but I cant see what is wrong with the schema.

source/simulation2/components/CCmpSelectable.cpp
184 ↗(On Diff #12700)

oh, thnx

114 ↗(On Diff #12640)

I think it refers to in game length (with scaling), like attack range is somewhere said it is in meters or something like that.

In D2844#124275, @Angen wrote:

I've been unable to use Shape with Outline (template validation issue?)

I am not sure what is wrong with it. It worked in previous diff and all I did was to surround choice with Shape and moved it inside overlay.
It looks like it does not look at the schema ? or schema is somewhat wrong but I cant see what is wrong with the schema.

Perhaps you're missing an "interleave"? I'm not sure if order is important.

Angen added a comment.Jul 18 2020, 5:17 PM

Perhaps you're missing an "interleave"?

Oh it was that one.
But I do not get the fact there have already been two nodes inside overlay and it worked just fine.

Angen updated this revision to Diff 12765.Jul 18 2020, 5:25 PM

interleave
rename variables

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpSelectable.cpp
|  71| »   DEFAULT_COMPONENT_ALLOCATOR(Selectable)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If DEFAULT_COMPONENT_ALLOCATOR is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2696/display/redirect

wraitii requested changes to this revision.Jul 20 2020, 11:15 AM

This works much more nicely :)

One final issue -> when using Outline, you have to initialise the fpShape, otherwise I fear compilation might make it default to "not circle", which means circle outlines don't get drawn. See inline.

source/simulation2/components/CCmpSelectable.cpp
536 ↗(On Diff #12765)

You need to initialise this value:

ICmpFootprint::EShape fpShape = ICmpFootprint::CIRCLE;

I've tested with a circle outline and it drew a box in your code. I suspect it compiles to a check like "fpShape != 0", and since this will be a random value, it's rather unlikely to be 0.

This revision now requires changes to proceed.Jul 20 2020, 11:15 AM
Angen added inline comments.Jul 20 2020, 11:38 AM
source/simulation2/components/CCmpSelectable.cpp
514 ↗(On Diff #12765)

and this

588 ↗(On Diff #12765)

actually I should rather fix this if

Angen updated this revision to Diff 12822.Jul 20 2020, 6:08 PM

final fixes

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/simulation2/components/CCmpSelectable.cpp
|  71| »   DEFAULT_COMPONENT_ALLOCATOR(Selectable)
|    | [MAJOR] CPPCheckBear (unknownMacro):
|    | There is an unknown macro here somewhere. Configuration is required. If DEFAULT_COMPONENT_ALLOCATOR is a macro then please configure it.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/2732/display/redirect

wraitii accepted this revision.Jul 25 2020, 6:25 PM

Works nicely now.

I think this is a very good change, thanks for the work :)

This revision is now accepted and ready to land.Jul 25 2020, 6:25 PM