What would be the correct fix in your opinion.

Apparently territory_pull.xml is supposed to be placeable, and now it no longer is. How about rallypoint.xml and target_market.xml, they're in special/ too, but should they be unplaceable? 

@fatherbushido, do you have any advice for us?

Options I see for territory_pull.xml:

• move it back into other/, which contains only structures
• move it into trigger/, which typically inherit from template_trigger_point.xml
• move it into a new folder for special things that are placeable, e.g. miscellaneous/
• move it one level down simply into templates/

These all have varying degrees of ugliness, however.

Also, I agree with @elexis' suggestion to rename special/ to unplaceable/; that's separate from solving the territory pull, though.
See also D877#34302, D1798#77705, and #4770.

I'd vote for unplaceable/, moving the three files you mentioned somewhere else and fixing the error when placing territory_pull.

somewhere else

Where?

unplaceable/

Also, where are the Atlas filters defined?

Perhaps a more elegant solution would be to make territory_pull.xml a child of template_trigger_point.xml; comparing the files, they seem to have many lines in common.

In rP22266#33466, @Nescio wrote:

somewhere else

Where?

Reasoning should usually be:

Step 1: Fact gathering stage

• You mentioned some choices.
• I don't know if imagination was exhausted or if one can brainstorm more alternative folder structeres.
• Perhaps in some ticket or irclogs one could find more information, and in the commit log if we learnt anything from the concern.

Step 2:

• Define the problem that you want to solve, use cases and identify the stakeholders

Step 3:

• Compare the advantages and disadvantages of the different approaches
• Try to construct a solution that covers all advantages of all solution while dropping all disadvantages of the approahes

(If I'm not mistaken that's thesis, antithesis, synthesis.)

There was some quote that describing the problem precisely will ultimately reveal the solution while doing so.

If such an optimum is found (there can be multiple) and proposed, then the reviewer doesn't have any choice other than to accept after read (since the proof of correctness is valid and was read by the reviewer).
This is the fastest way around the review barrier if there is noone interested in doing the work to establish the facts, define the problem and determine the solution but have someone else make a patch for it.
If a contributor shows that he is capable to take well educated decisions on their own, they should also get commit access (that is if a volunteer organization wants to be efficient and effective in reaching their objectives)

To say something on topic, I agree that the "other/" folder is indescriptive and should be deprecated, i.e. not used for future commits except removal from that folder.

I could say more now where I thought about this, but I'd like to hear your best shot first :->

unplaceable/

Also, where are the Atlas filters defined?

You linked it in your last post :-)

Problems identified in earlier discussions:

• other/ is confusing and ought to be deprecated (move structures there to structures/)
• special/ is confusing and ought to be renamed to unplaceable/
• territory_pull.xml should actually be placeable in Atlas
• prior to this commit, territory_pull.xml caused errors when attempting to place it in Atlas

The concern raised is specifically about the third point, so that should be addressed first. Fundamentally the options are:

• move it to an existing folder
• move it to a new folder
• keep it where it is, but change the Atlas filters (again)

The fourth point requires simply inserting the following lines (cf. rP17270 and rP22070):

<Visibility>
  <AlwaysVisible>false</AlwaysVisible>
  <Corpse>false</Corpse>
  <Preview>false</Preview>
  <RetainInFog>false</RetainInFog>
</Visibility>

Concerning the first and second points, it's clear what is to be done, but because they affect numerous files, it should probably be split up in several patches, to make it easier to review and check for errors.

unplaceable/

Also, where are the Atlas filters defined?

You linked it in your last post :-)

Well, I couldn't find it when I did a grep in the public/ folder.

(If I'm not mistaken that's thesis, antithesis, synthesis.)

Not exactly; antithesis is the negation of the thesis, synthesis a way to reconcile the two. (Contrary to popular belief, it's actually Fichte's, not Hegel's.) E.g.:

• Progress is driven by capitalism
• Progress is not driven by capitalism
• Progress is sometimes partially driven by capitalism

Well, I couldn't find it when I did a grep in the public/ folder.

Which means it might not be defined in that folder.

First bullet point in the post you linked above contains the answer https://code.wildfiregames.com/D1798#77705

In general Atlas source is in source/tools/atlas/, but it loads the templates via the TemplateManager as mentioned over there, i.e. it's hardcoded in C++.

But don't forget your initial thought / assumption that it's in some easily editable data file (especially changeable without compiling).
Often the first thought or assumption is shared by the other naive users as well, which means the dev may consider making it more intuitive for more users by removing that hardcoding.
(It's an idea that can be developed and might or might not be good.)

So if we want to rename special/, we have to replace that one C++ file, compile it again (BuildInstructions).
Not to forget checking whether there are more than this one hardcoding of the folder.

So much about the https://en.wikipedia.org/wiki/Requirements_analysis, but there is also the https://en.wikipedia.org/wiki/Stakeholder_analysis.
The author who moved the file in question out of this directory is one of the stakeholders too, so perhaps one can ask him if he had different plans with that file, or objects to a rename and thus diverges the two repositories further.

Which means it might not be defined in that folder.

Yeah, I figured as much, hence my earlier question.
Undoubtedly there is a reason why it's hardcoded in some C++ file. However, it also means that if one would want to create a game or mod organized differently than 0 A.D.'s public folder, they would have to change the core engine as well.
Ideally there would be an Atlas settings file directly in the public folder (cf. mod.json), giving more flexibility. I'm not a programmer, though, and have no idea how feasible that would be; presumably it's not important enough to be worth the hassle.

Well actually there are some files in binaries/data/tools/atlas for atlas, that I assume, could easily be moved to the modmod.

Anyway, that's not directly related to the concern raised here.
Earlier (rP22266#33467) I suggested to make territory_pull.xml a child of template_trigger_point.xml, which has the following advantages:

• it is placeable in Atlas again
• it solved the visibility errors
• it makes it clear where it should be moved to (trigger/)

I'm not saying it's the only option, but right now it seems the least ugly one to me. I'm open to criticisms and other suggestions, of course. It's certainly possible I'm overlooking things.

In rP22266#33489, @Nescio wrote:

Which means it might not be defined in that folder.

Yeah, I figured as much, hence my earlier question.

(It could also be that it's in the folder but you didn't find it with grep)

Undoubtedly there is a reason why it's hardcoded in some C++ file.

(Presumably because it required less effort, it's not trivial where to put it if not into that C++ function. The commit that introduced it linked was the simulation rewrite by Philip, which contained hundreds of things.)

it also means that if one would want to create a game or mod organized differently than 0 A.D.'s public folder, they would have to change the core engine as well.

(Unlikely that someone wants to use a different unplaceable/ folder, or wants to put placeable things in there if he knows that that is its purpose. The important part is education about it's purpose)

presumably it's not important enough to be worth the hassle.

Yes

But also see my referenced comment that this is not Atlas specific, Atlas uses the TemplateManager, and the TemplateManager decides what placeable templates to return.
There can/could be callers other than Atlas too, for example random map scripts, the simulation or AIs might call this "get all templates" function.
(Perhaps the function should also be renamed to "GetAllPlaceableTemplates" or similar.)

territory_pull.xml a child of template_trigger_point.xml

Might fix the errors, but does the inheritance make sense?
A trigger point is an entity that is to be used by a triggerscript, but territory_pull is not going to be used by triggerscripts, just supposed to modify the territory weights.
Most importantly they have the TriggerPoint component which this one doesn't have.
There is more data on tickets etc. what trigger points are / meant to be / meant to have become, but I guess the inheritance can't really be justified?

I guess you proposed to put it into trigger/ because that solves the problem of where to put this weird template.

This file seems to be out of place everywhere:

• in the root folder because that only contains template_*.xml files
• the existing folders have a different purpose
• a new folder means only that one file would be in it
• other/ would fit technically, but it would be the only one remaining in that folder, and other/ is equally dangerously ambigous

The folder and filename should specify what it contains, so describing the essence of the file and abstracting a bit should yield us a good foldername.
The file modifies the terrain properties.
Abstracting it: The file modifies properties of nearby simulation components.
Delenda Est has some entities that have an aura to modify the gatherrate of fields, so that would be a similar template that might be useful in the same folder.
He put it into the other/ folder: https://github.com/JustusAvramenko/delenda_est/tree/master/simulation/templates/other
I'm thinking of "modifiers/", but I'm grasping at straws :-/

@wowgetoffyourcellphone got an idea for a folder name where entities like your field aura entities from delenda est, or territory_pull.xml (that changes the territory weight, from https://trac.wildfiregames.com/changeset/11400) could be placed? A name that refers to these kinds of entities?

I guess you proposed to put it into trigger/ because that solves the problem of where to put this weird template.

Actually I wanted to figure out why triggers don't cause errors in Atlas, so I opened the template_trigger_point.xml and territory_pull.xml files next to each other and compared them. Then I noticed they share the majority of lines and territory_pull.xml could be simplified to:

<?xml version="1.0" encoding="utf-8"?>
<Entity parent="template_trigger_point">
  <Capturable>
    <CapturePoints>5</CapturePoints>
    <GarrisonRegenRate>0</GarrisonRegenRate>
    <RegenRate>0</RegenRate>
  </Capturable>
  <TerritoryDecay>
    <DecayRate>5</DecayRate>
  </TerritoryDecay>
  <TerritoryInfluence>
    <Root>false</Root>
    <Radius>32</Radius>
    <Weight>30000</Weight>
  </TerritoryInfluence>
  <VisualActor>
    <Actor>props/special/common/waypoint_flag.xml</Actor>
  </VisualActor>
</Entity>

(19 lines instead of 45). And because of its new parent, it's clear where it would have to be moved to then.

However, you're right this approach won't necessarily work for miscellaneous other special placeable files.

This file seems to be out of place everywhere:

Yes, it does.

trigger_point.xml may not have the TriggerPoint component, but all the files inheriting it do and the filename indicates that.

Some superficial research on what a Trigger, and thus a TriggerPoint is:

• https://trac.wildfiregames.com/wiki/Triggers - mostly mapscripts
• https://en.wikipedia.org/wiki/Trigger - "event-driven programming"
• https://trac.wildfiregames.com/ticket/52
• https://trac.wildfiregames.com/ticket/53
• https://wildfiregames.com/forum/index.php?/topic/18211-discussing-triggers/ - 2014 "Triggers" are JavaScript codes that are run in the simulation

So it mostly relates to scripts that are run after the simulation started that are relate to simulation events.

territory_pull doesn't add any custom simulation code, doesn't relate to events in the game any further than entity templates, auras and techs do.
So if that trigger_point file was chosen as a parent, it would have to be rebranded, it doesn't really fit.

With regards to the parent sharing some code, without further evidence I would guess that this is attributable to the method of copy&paste.

Historically people recommended to commit a revert first and then start thinking again, but such things don't add information to the svn log if one intends to change it afterwards yet again, so I would recommend to revert this unless we/you conclude a correct solution to the sorting of this file.

One problem to finding a good folder for this file is because the file is very peculiar, there are no comparable examples in vanilla. Hence I was looking for more examples that allow finding a more common denominator. I could only think of the aura entities in Delenda Est.

I suppose it's a modifier entity, so perhaps templates/modifiers/, but that's just slightly less random than a random guess. Better would be reading 16 years of chatlogs and forum post history, comparing with mods and basing a decision on that, or otherwise producing evidence that leads to the discovery of a solution that satisfies the technical needs and the intention of your original diff.

As mentioned in the PM that got me into reviewing this to begin with, it would be good to coordinate this with the other repository. You already tried here, but at least I'm deterred by perpetual merge conflicts and diverging codebases.

Yeah, the whole situation is quite annoying; as has been frequently pointed out already, territory_pull.xml is unlike anything else. It has a location, territory influence, and capture points (only structures do), yet it has no cost or health (so it's clearly not a structure either).

Let's sum up our options:

• keep it where it is: unacceptable, because it should be placeable
• move it one step back, directly into templates/: unacceptable, because unprecedented, only template_* files are there
• move it into an existing folder:
  • units/: unacceptable, because it's not an unit
  • trigger/: unacceptable, because it doesn't have a trigger component
  • structures/: unacceptable, because it's not a structure
  • skirmish/: unacceptable, because it's not a placeholder for skirmish maps
  • rubble/: unacceptable, because it's not rubble
  • other/: this is where it came from; folder's name and existence is confusing, ought to be deprecated
  • gaia/: no reason to move it there
  • campaigns/: what's the purpose of this? (contains three structures and three units)
• create a new folder and move it there (miscellaneous/?)

misc is synonymous with other, nothing gained.
template/modifiers/ from the last two posts is not on the menu? Ambiguosity and missing masterplan speak against it, what else on top of that?
We can find more examples than the field aura entities, especially if we take a look at the simulation components available and other weird but not special templates of mods; and from that we might find a common denominator that may be more specific than templates/modifiers/.
TriggerPoints didn't seem so bad, just that it's a bit in conflict with TriggerPoint simulation component connotation. Maybe template/modifier_points/, template/effect_points/ or something if it can be known that all modifier entities have a Position component.

But what does it modify?

Here are my two cents here. In an old game I liked (Die Siedler 5, das Erbe der Könige), There was a special Misc category, for special entites such as triggers, or ambient sounds origins.
Now that @elexis ruled Misc out for reasons of clarity, maybe we could have a 'templates/scenario/' folder for everything that can be used for scenarios which have no place elsewhere.

In rP22266#33606, @Nescio wrote:

But what does it modify?

It does not modify the property of an entity component but it does modify the TerritoryInfluence weight at the position of the entity, correct? Gatherrate for the field aura entities in DE.

In rP22266#33607, @Stan wrote:

Here are my two cents here. In an old game I liked (Die Siedler 5, das Erbe der Könige), There was a special Misc category, for special entites such as triggers, or ambient sounds origins.
Now that @elexis ruled Misc out for reasons of clarity, maybe we could have a 'templates/scenario/' folder for everything that can be used for scenarios which have no place elsewhere.

Sounds like other/

If we can construct one reasonable entity template that doesn't have some kind of effect, then other/ isn't to be deprecated.

In rP22266#33608, @elexis wrote:

If we can construct one reasonable entity template that doesn't have some kind of effect, then other/ isn't to be deprecated.

What about, a local sound emitter, an object that reveals to one ore more players the given radius at a specific location (ie placeable bonfires, could even have a decreasing vision range over time). Those are all useful things that can be put in the other/ folder.

It does not modify the property of an entity component but it does modify the TerritoryInfluence weight at the position of the entity, correct?

The things that spring to me my when reading modifier are auras, technologies, and maybe those special/player/ files; territory_pull.xml is not more a modifier than a structure is.

Gatherrate for the field aura entities in DE.

Those field entities inherit from template_gaia_farmland.xml so properly ought to be under gaia/ or farmland/, not other/.

maybe we could have a 'templates/scenario/' folder for everything that can be used for scenarios which have no place elsewhere.

We already have campaigns/ and skirmish/ folders; don't you think adding a scenario/ folder would add more confusion?

If we can construct one reasonable entity template that doesn't have some kind of effect, then other/ isn't to be deprecated.

The problem is that other can mean about anything and nothing. If our purpose is simply to solve the concern raised, then yes, moving territory_pull.xml back into other/ would do that; however, to me that feels more like postponing a problem rather than a real solution (having unambiguous folders where it's immediately clear where a new file should go to).

`<VisibleInAtlasOnly> is true for territory_pull, the delenda est field templates, but false all structures, unless a structure that is invisible ingame would be implemented. But if it was invisible and thus have no visible actor to the player, how could it qualify as a structure?

Entities which have VisibleInAtlasOnly true must have a Position component, and if they are invisible ingame, the only use case I can imagine is their effect on the simulated environment, thus justifying modifier_points or effect_points, or trigger_points where TriggerPoint is rebranded and redefined somehow (possibly leading to confusion as opposed to a new title the desired class of entities to be sorted to a new/different directory).

No, I'm not saying territory_pull.xml is a structure, certainly not. However, I don't think it qualifies as a modifier either; if it does because it has a territory influence, then all entities that have a territory component would qualify as modifiers too; (in)visibility doesn't change its modifying properties.
The core issue is that something which inherits from template_*.xml goes into */, something which should be unplaceable goes into special/, but it's unclear what to do with something which doesn't inherit from any template but is supposed to be placeable; currently territory_pull.xml is the only such file, but who knows, there might be more miscellaneous files in the future.
Unfortunately, I don't have a real solution; I only see problems with all options proposed so far.

IMHO

Special

Placeable
Unplaceable

would be nice.

I don't think it qualifies as a modifier either; if it does because it has a territory influence, then all entities that have a territory component would qualify as modifiers too; (in)visibility doesn't change its modifying properties.

The argument for modifiers/ of effects/ would be that their essence is the effect and only the effect, whereas the essence of the structure is the matter represented by the VisualActor, optionally having an effect.

As mentioned one could use the qualifier VisibleInAtlas = true as a definition, for instance templates/VisibleInAtlasOnly.../

template_trigger_point.xml: <VisibleInAtlasOnly>true</VisibleInAtlasOnly>
special/territory_pull.xml: <VisibleInAtlasOnly>true</VisibleInAtlasOnly>

but then the triggerpoint templates would have to be moved as well for consistency.

I suppose the special/ templates however could also have this property but should remain unplaceable.

The core issue is that something which inherits from template_*.xml goes into */

So that's the masterplan and the file is template_territory_pull.xml, is that what's going? But then the files inheriting that (for instance different territory weights) still need a different folder, eh templates/other/territory_pull_50.xml...

Looking at further examples that can be discovered without doing any research, from the other/ folder in DE (https://github.com/JustusAvramenko/delenda_est/tree/master/simulation/templates/other), those templates should be in templates/, templates/gaia/.

So I only see something like templates/other/ or templates/placeable_but_invisible/ or templates/modifiers/ along templates/unplaceable/ or templates/special/ for (templates inheriting) territory_pull, the DE field entities and any placeable template that is invisible;

Or not having the other/ folder but, assuming there were many such placeable invisible entities inheriting a generic template, sorting them into templates/farmlands/farmland_XX.xml, templates/territory_pull_XX.xml/ instead of templates/something/....xml.

Placeable
Unplaceable

Most things are placeable, so that would imply units etc should be moved into there; I don't think that would be an improvement.

The core issue is that something which inherits from template_*.xml goes into */

So that's the masterplan and the file is template_territory_pull.xml, is that what's going? But then the files inheriting that (for instance different territory weights) still need a different folder, eh templates/other/territory_pull_50.xml...

What do you mean? I don't quite understand. It's clear what's inside rubble/, structures/, triggers/, units/: files that inherit from template_rubble.xml, template_structure.xml, template_trigger_point.xml, template_unit.xml, respectively.
The trouble is territory_pull.xml doesn't inherit from anything and thus doesn't have an obvious place to go. That doesn't mean we should create a template_territory_pull.xml and a territory_pull/ folder, that would be non-sensical file inflation.

→ Most things are placeable, so that would imply units etc should be moved into there; I don't think that would be an improvement.

No that would be like have a template named special_placeable_trigger_point and special_unplaceable_whatever and no folders. Not adding special_placeable to everything

In rP22266#33457, @Nescio wrote:

@fatherbushido, do you have any advice for us?

I must finish the notes I promised you before :)

@elexis did you get the notes ? :)

No notes.

That these templates shall not appear in the list of placeable templates may be true, but there are also the generic templates that should not appear in that list.
Also that a template shall not appear to be placeable doesn't mean that it is impossible to place it if it would appear in the list.
Hence unplaceable/ would also be imperfect.

Another possibility would be hidden/, but that also is ambiguous. Everything is ambiguous.
Perhaps unplaceable/ and a README.txt defining unplaceable/ as not appearing in the Atlas list of entities would be the solution that satisfies all problems (including the clarification that a template that may technically be placed may be contained in that folder).

Regardless of whether and how special/ is renamed, territory_pull.xml must be moved to a different folder.

Following previous comments, the possibility to have multiple territory pull entities with different numbers, similar to the delenda est farmland auras, it seems reasonable to have an other/ folder, which anyone may propose to rename (haven't seen anything better) and to put these files there.

If the README.txt is added and the special/ folder renamed, that should be an improvement on transparency and correctness.

Also wondering if the contents of special/ should be sorted a bit more recognizably, for instance rallypoint.xml, target_marker.xml might be considered some GUI subclass of the unplaceables folder.

In rP22266#34230, @elexis wrote:

No notes.

That these templates shall not appear in the list of placeable templates may be true, but there are also the generic templates that should not appear in that list.
Also that a template shall not appear to be placeable doesn't mean that it is impossible to place it if it would appear in the list.
Hence unplaceable/ would also be imperfect.

Another possibility would be hidden/, but that also is ambiguous. Everything is ambiguous.
Perhaps unplaceable/ and a README.txt defining unplaceable/ as not appearing in the Atlas list of entities would be the solution that satisfies all problems (including the clarification that a template that may technically be placed may be contained in that folder).

Sucks to be wasting so much time on a stupid template that's basically not being used...

@Nescio renaming to template_territory_pull.xml would solve the problem without adding the README.txt complexity, and it wouldn't be a cheat because delenda est can continue to use a custom other/ or rename to myfieldsthing/, and if someone was to add more territory pull child templates to vanilla, they may be added to a territory_pull/, which is still better than other/ it seems. Make this red go away.

Thanks! However, template_*.xml files don't show up in Atlas either, so it wouldn't actually solve the issue? Or do you mean creating additional files?

template_territory_pull.xml
territory_pulls/32.xml

That or keeping other/.