Page MenuHomeWildfire Games

move territory_pull.xml from other/ to special/
ClosedPublic

Authored by Nescio on Mar 19 2019, 8:31 PM.

Details

Summary

This patch moves territory_pull.xml from other/ to special/ and updates the only file in which it is currently used (maps/scenarios/Campaign Test Map.xml), because it is something special and thus doesn't belong in other/ (which contains various gaia structures).

Test Plan

Ought to be unproblematic.

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

Nescio created this revision.Mar 19 2019, 8:31 PM
Owners added a subscriber: Restricted Owners Package.Mar 19 2019, 8:31 PM
Vulcan added a subscriber: Vulcan.Mar 19 2019, 8:32 PM

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

Link to build: https://jenkins.wildfiregames.com/job/differential/1123/display/redirect

elexis accepted this revision.Apr 3 2019, 8:07 AM
elexis added subscribers: Stan, elexis.

doesn't belong in other/ (which contains various gaia structures).

Confirmed, looked at all the other files in there.
Makes me wonder why that folder is called "other" and notthing relating to "gaia"?
So should the "other" folder be moved to gaia afterwards?

because it is something special

What makes it special though. The absence of clear definitions of "other" and "special" is what has led to the wrong sorting to begin with.
This entity does not present a VisualActor to the player, perhaps thats the common denominator of the special folder?
Rallypoint and TargetMarker have one, but perhaps they are... other special.

At last, we should check whether the move is not only correct, not only ideal, but also whether deleting it wouldn't be better.
For example if the template doesn't even work and cannot work, it would be better to delete it.

While trying to test it, I encountered the same heightmap without entities on another scenario campaign demo map, the latter should be deleted, I forgot the filename, anyway.

I managed to test the entity in Atlas by dragging the waypoint flags around and seeing that it modifies the territory.

So it's an interesting gameplay element, it allows the map author to change the relevance of certain areas of the map, even though it doesn't provide a mechanism to communicate that to the player. I imagine the importance of that territory could be embedded into the storyline.

The template comes from rP9889, the map from rP11400 supposedly.
Therefore the patch is not only acceptable but must be accepted.

For future patches, I would recommend you to try to make an irrefutable case as to why the patch is correct (the reader should not have a way to ask "Why?", here: why special is the right folder)

Perhaps @Stan wants to commit it.

This revision is now accepted and ready to land.Apr 3 2019, 8:07 AM

Thank you for your review.

What makes it special though.

Let's have a look at what's in the different simulation/templates/ folders:

  • everything under rubble/ inherits from template_rubble.xml
  • everything under trigger/ inherits from template_trigger_point.xml
  • everything under units/ inherits from template_unit.xml

What's so special about the special/ folder is that what's in there (except for formations) does not inherit from any of the generic template_*.xml files.
other/territory_pull.xml is special because it's unlike anything else and has no parents.

Makes me wonder why that folder is called "other" and notthing relating to "gaia"?
So should the "other" folder be moved to gaia afterwards?

The other/ folder contained a lot of different things under A22, but since then gaia objects (ruins, treasures) have been moved to gaia/ and units (catafalques, plane) to units/. I agree the existence of the other/ folder is confusing and that it would be better to deprecate it. Because everything what's left under other/ inherits from template_structure.xml, it would make sense to move all those other/ files to structures/ in a future patch.

special/ does not inherit from any template_*.xml

So
special/ => doesn't inherit template_*.xml

One may wonder whether the converse is true too, i.e.
special/ <= doesn't inherit template_*.xml

(Or not special/ => does inherit template_*.xml)

If so, it would be a precise characterization of the files that belong there, possibly the definition of the folder.

What word in the dictionary that expresses that?
I'm thinking of something along the lines of "unqiue", but I'm sure there is some catchy word that had made sense all along but noone thought about it yet.

The other/ folder contained a lot of different things under A22, but since then gaia objects (ruins, treasures) have been moved to gaia/ and units (catafalques, plane) to units/. I agree the existence of the other/ folder is confusing and that it would be better to deprecate it. Because everything what's left under other/ inherits from template_structure.xml, it would make sense to move all those other/ files to structures/ in a future patch.

Deal

Actually it's the other way around: with this patch, everything that doesn't inherit from a template_*.xml is inside special/
The reverse is no longer true: D933 moved formations (which inherit from template_formation.xml) into the special/ folder.

elexis added a comment.Apr 3 2019, 1:49 PM

Hm. I think auras and techs are also templates, just not entity templates.

Perhaps it could become something like:

templates/R-entities/
templates/S-entities/
templates/formations/
templates/techs/
templates/auras/

where R are the units and structures, and S the special ones, just with somehow more descriptive names.

Or

templates/entities/units/
templates/entities/structures/
templates/entities/formations/
templates/entities/special/
templates/techs/
templates/auras/

With templates/entities/ containing no file, except in subdirectories.

Dunno, thoughts. Should certainly agree on this before possibly having wsated a lot of time moving files around and later changing ones mind (especially since phabricator like to not adapt moved files well, marking it as a new file and deleted previous file when committing; or I was doing it wrong)

Nescio added a comment.Apr 3 2019, 2:08 PM

Hm. I think auras and techs are also templates, just not entity templates.

What do you mean exactly? Auras and technologies are json files located under simulation/data/auras/ and simulation/data/technologies/, respectively; why should they be placed under simulation/templates/ instead?

elexis added a comment.EditedApr 3 2019, 2:21 PM
In D1798#73947, @Nescio wrote:

Hm. I think auras and techs are also templates, just not entity templates.

What do you mean exactly? Auras and technologies are json files located under simulation/data/auras/ and simulation/data/technologies/, respectively; why should they be placed under simulation/templates/ instead?

Whether the content is expressed in JSON or XML is not so relevant IMO. Notice we also have XML and RNG files in data/, and people navigate folders depending on their meaning.

The difference is that a tech/aura description only relates to exactly one technology, whereas one entity template relates to arbitrarily many entities of the same template, and also allows inheritance.
So you're right, aura/techs aren't templates in that sense.

The commonality between tech/aura files and entity templates is that they specify the properties of a simulation thing in the game (contrary to data/settings/ files for example).
I guess the commonality is that they are production queue item descriptions.
Edit: except for auras, meh

Nescio added a comment.Apr 3 2019, 3:01 PM

Still, what's wrong with the current location of auras and technologies?

elexis added a comment.Apr 3 2019, 3:18 PM

I guess every improvement can be defined as a defect. Files should be grouped by commonalities, right? The less fragmented it is, the better. I didn't claim this to be a good idea, was just exploring thoughts. I'm sure the settings/data/ folder was once grouped by the fact that it's JSON.

Nescio added a comment.EditedApr 3 2019, 4:23 PM

The organization of file locations (and names) should be logical and easy to understand, to keep things simple for future additions and mods; if one creates a new file, it ought to be immediately clear where to put it.
I think it's not really important whether auras are located under data/, under templates/, or somewhere else in the simulation/ folder; the important thing is that all aura files are inside auras/.

Since we're now discussing things beyond the scope of this patch, it might be worth having a critical look at how the simulation/templates/ folder is currently organized:

templates/
  campaigns/
  gaia/
    ruins/
    treasure/
  other/
  rubble/
  skirmish/
    structures/
    units/
  special/
    filter/
    formations/
    player/
  structures/
  trigger/
  units/
  • as discussed earlier, other/ can be deprecated and its files moved to structures/
  • campaigns/ contains just six files, three units and three structures, so perhaps those files could be moved accordingly
  • skirmish/structures/ might be moved to structures/skirmish/ and skirmish/units/ to units/skirmish/

If the above three points would be implemented, we would have:

templates/
  gaia/
    ruins/
    treasure/
  rubble/
  special/
    filter/
    formations/
    player/
  structures/
    skirmish/
  trigger/
  units/
    skirmish/

The advantage of this is that folders would then roughly correspond to matching templates:

  • does not inherit from any template_*.xml file ⇒ special/
  • inherits from template_formation.xml ⇔ special/formations/
  • inherits from template_bird.xml, template_gaia.xml, or template_unit_fauna.xml ⇔ gaia/
  • inherits from template_rubble.xml ⇔ rubble/
  • inherits from template_structure.xml or template_wallset.xml ⇔ structures/
  • inherits from template_trigger_point.xml ⇔ trigger/
  • inherits from template_unit.xml but not template_unit_fauna.xml ⇔ units/

(It is also possible to replace template_unit_fauna.xml with template_gaia_fauna.xml (as I've done here) or to move fauna from gaia/ to units/fauna/ to improve consistency.)

Furthermore, it might be worth considering creating subfolders for each civilization, i.e. move {civ}_*.xml to {civ}/*.xml. The obvious disadvantage is that it involves moving hundreds of templates and changing thousands of lines in the maps/ and simulation/ folders. The advantage is that templates would then correspond to the actor files, which are already organized in separate folders per civilization (e.g. art/actors/structures/athenians/ and simulation/templates/structures/athen/).

Stan accepted this revision.Fri, May 10, 8:45 AM
This revision was automatically updated to reflect the committed changes.

does not inherit from any template_*.xml file ⇒ special/

The argument doesn't seem valid.

The argument doesn't seem valid.

Forgive me my ignorance, but why not?

! In D1798#77442, @Nescio wrote:
Forgive me my ignorance, but why not?

I am not in charge of that svn repo. I won't step on someone's toes. ;-)

does not inherit from any template_*.xml file ⇒ special/

I guess one can easily find an example for a template that would make sense outside of special/ and would not inherit from any template_*.xml.

Nescio made the statement only on the templates in the current templates folder:

The advantage of this is that folders would then roughly correspond to matching templates:

does not inherit from any template_*.xml file ⇒ special/

But it's indeed relevant for future and other mods as well to get a clear definition of what the folders should and should not contain.
Unless there is somewhere an official definition, it is subject to arbitrariness one way or another.
I based my argument to move it on the fact that this doesn't have a VisualActor as a commonality. But one could also argue for different directory structures (splitting the special/ folder, doing something with trigger/.
It seems some templates are sorted depending on their values (units, structures), at other times depending on how they are used by pyrogenesis (special/). That could become more consistent, maybe.
This acceptance shall not express belief of superior knowledge on the issue more than a kind response to a kind request and is open to revision.
The only toe is in templates/ is in the trigger/ folder.

In D1798#77495, @elexis wrote:

To avoid misinterpretation (there weren't in the above message): "step on someone's toes" is a saying meaning I didn't want to involve myself in that (nor to belittle anybody). But now, I will be classified as cryptic if I don't say more or (insert the adjective) if I give more details. You know me enough to know that I prefer shut up instead of having to proove things ;-)
73

(Only non-hypothetical statements can be substantiated by a proof.
One can use hypotheticals and conditionals to weaken force of expression,
and questions don't strictly make any claims at all.

I don't know you enough to understand all your actions or messages, but I've never seen you use swearwords and rarely condescending tone, so I don't see you belittling anyone.
(insert the adjective) if I give more details is satisfied adjective == helpful if the feedback is constructive.

Since you posted, you wanted to convey some information to Nescio, Nescio wants to know what you mean, so noone has any reason to complain if you convey your thoughts in a constructive manner,
and noone has reason to ask you to substantiate your thoughts if they are not assertive.)

In D1798#73886, @elexis wrote:

The test plan is "Ought to be unproblematic."
It was commited in rP22266 with a commit message containing an assertion which was questionned by a reviewer (you).
A very quick look at commit history leads to flashing "special/ should not contain placeable templates".
The template was placed in a scenario map (using atlas?). Now try to place it in atlas (I won't check, I haven't a wfg-a24 svn ready).
I have no more money so I must cut here the telegraph.
73

elexis added a comment.EditedSat, May 11, 5:20 PM

A very quick look at commit history leads to flashing "special/ should not contain placeable templates".

Thanks, indeed I forgot D877, but I'm glad I pointed out that the directory should be renamed to reflect the meaning. (Edit: here too https://trac.wildfiregames.com/ticket/4770#comment:4)

I have no more money so I must cut here the telegraph.

Wildfire Games is furthered by SPI for non-profit purposes, so you might be compensated under an amount that would ordinarily be paid for like services by like organizations in like circumstances.

I have no more money so I must cut here the telegraph.

Do you refer to what I said in that meeting I referenced in https://wildfiregames.com/meetinglogs/logs/0ad-staff.2019-04-07-18.12.log.html

18:14:03 <elexis> #info elexis in december 2018 succeeded to pull out of active development after each of the the last five releases revealed more that three working conditions are not acceptable for me in order to continue working fulltime on the project throughout the year.
18:26:46 <elexis> most of my revision history would not have been possible with a review policy, it's mathmatically impossible to work more than 5 years full time without any prospect for compensation, and it's obviously unsatisfying if people leave the team in rage

Thank you for your clarification, @fatherbushido .
If the special/ folder is supposed to only contain unplaceable things, then perhaps it ought to be renamed to unplaceable/.
Also, the other files in the other/ folder should probably be under structures/, because they're structures.
As @elexis pointed out earlier, the existence and name of this folder is quite confusing.

I suppose there is no difference between the entities that shall be placeable in Atlas and entities that can be placed on maps without Atlas.

Just renaming to "unplaceable" clarifies that the folder shall not contain any unplaceable templates, which is progress, but still doesn't inform anyone which templates are considered placeable.

  • Atlas uses FindTemplates from TemplateLoader.cpp to determine which templates are shown in the list of placeable entity templates. This calls AddToTemplates which shows the special/ filter and the other filter not mentioned in the special discussions (which appears to come from rP7259):

// We want to ignore template_*.xml templates, since they should never be built in the editor

As to the reasons why the templates ignored by this TemplateLoader function are not placeable:

  • filter/ templates might be insufficient to create an entity
  • actor.xml is used similarly to a filter template
  • A formations/ entity may only exist if there is at least one other entity that is a member of this formation, therefore entities with a Formation component may not be placed in maps
  • player/ entities don't have a position component, so they cannot be placed
  • territory_pull is supposed to be placeable.
  • rallypoint.xml and target_marker.xml are GUI visualizations, arguably placeable
  • spy.xml I forgot about that.
  • template_*.xml templates may be very well placeable technically and some template authors may wonder why their templates aren't listed.

Template editors should not be familiar with C++ implementation details.
The C++ code could become so smart that it filters exactly the templates that are impossible to be placed,
but that would require loading all file contents and parsing them, so I agree that this would be preferably avoidable complexity and performance cost.
I guess a README.txt in that folder is also preferably avoided.
So at least a rename to "unplaceable/" should be done as far as I can conclude.
However the template_*.xml files also aren't placeable, so if that folder was called unplaceable, then it's contradictory that these files aren't part of that folder (equally special is everything and nothing, so every and no file has a place there by that title).
But renaming it to unplaceable/ is still more descriptive than special/.
Given that not only special/ but also template_*.xml are special, a README.txt might actually do make sense.
It could also explain how filter templates can be implemented and conventions.
And having this in the source means it will also be accessible without browsing through websites of one provider.

I have no more money so I must cut here the telegraph.
now, I will be classified as cryptic if I don't say more

When I try placing a territory pull in Atlas in A23 (where it's located in other/) I get the following errors:

ERROR: RelaxNGValidator: Validation error: preview%7Cother/territory_pull:1: Expecting an element RetainInFog, got nothing
ERROR: RelaxNGValidator: Validation error: preview%7Cother/territory_pull:1: Invalid sequence in interleave
ERROR: RelaxNGValidator: Validation error: preview%7Cother/territory_pull:1: Element Visibility failed to validate content
ERROR: RelaxNGValidator: Validation failed for '(null)'
ERROR: Failed to validate entity template 'preview|other/territory_pull'

I get similar errors when selecting a wallset (located under structures/) in Atlas:

ERROR: RelaxNGValidator: Validation error: preview%7Cstructures/athen_wallset_stone:1: Expecting an element RetainInFog, got nothing
ERROR: RelaxNGValidator: Validation error: preview%7Cstructures/athen_wallset_stone:1: Invalid sequence in interleave
ERROR: RelaxNGValidator: Validation error: preview%7Cstructures/athen_wallset_stone:1: Element Visibility failed to validate content
ERROR: RelaxNGValidator: Validation failed for '(null)'

Are they actually supposed to be placeable?

Stan added a comment.Sun, May 12, 6:03 PM

I fixed the wallsets in svn IIRC. I'm not sure I fixed territory pull cause it's not visible by default.

Yes, I see you did: rP22070