Opened 5 years ago

Closed 3 years ago

Last modified 2 years ago

#2951 closed defect (fixed)

[PATCH] Dynamically generate templates for special entites

Reported by: Itms Owned by: Itms
Priority: Must Have Milestone: Alpha 22
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by elexis)

Currently, the generation of special templates ("preview|", "foundation|", etc.) is entirely hardcoded in TemplateLoader.cpp.

Several problems can arise, including the following:

  • This is bad for modding, obviously
  • Template validation is made difficult, especially in the case of program tests.

It would be nice to have a more generic extensible scriptable way to define subset templates.

Attachments (7)

template_subsets.diff (36.5 KB) - added by leper 4 years ago.
Done.
template_subsets_fix.diff (6.9 KB) - added by leper 4 years ago.
Incremental patch on top of the earlier one. Fixes issues found in the review.
actor_wip.diff (3.9 KB) - added by elexis 4 years ago.
Broken, but shows the approach (not convinced it's preferable)
2951_template_subsets_v2.diff (39.8 KB) - added by elexis 4 years ago.
merged github commits
commands_r18651.txt (1.3 MB) - added by elexis 4 years ago.
sample
2951_git_commits.7z (12.1 KB) - added by elexis 4 years ago.
Backup of the 14 git commits making up the committed cleanup and logic changing part for r18692.
2951_template_subsets_v2_without_committed_cleanup.diff (39.8 KB) - added by elexis 4 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 5 years ago by Itms

In 16033:

Remove testing of some special templates. We need to find a good way to generate template subsets, refs #2951.

This fixes the tests fail introduced in r16022.

comment:2 Changed 5 years ago by leper

#2711 was a duplicate of this.

comment:3 Changed 4 years ago by mimo

For some tests, i needed to stop the construction at a given value. There is the "Initial" value to do that, and when building the foundation template, the "Max" of the foundation should be put to the "Initial" of the origin entity. Maybe this should be added in that patch

Index: source/ps/TemplateLoader.cpp
===================================================================
--- source/ps/TemplateLoader.cpp        (révision 17780)
+++ source/ps/TemplateLoader.cpp        (copie de travail)
@@ -497,7 +497,9 @@
        // Add the Foundation component, to deal with the construction process
        CParamNode::LoadXMLString(out, "<Entity><Foundation/></Entity>");
 
-       // Initialise health to 1
+       // Initialise health to 1 and Max health to Initial health of base entity
+       if (in.GetChild("Entity").GetChild("Health").GetChild("Initial").IsOk())
+               CParamNode::LoadXMLString(out, ("<Entity><Health><Max>"+utf8_from_wstring(in.GetChild("Entity").GetChild("Health").GetChild
("Initial").ToString())+"</Max></Health></Entity>").c_str());
        CParamNode::LoadXMLString(out, "<Entity><Health><Initial>1</Initial></Health></Entity>");
 
        // Foundations shouldn't initially block unit movement

Changed 4 years ago by leper

Attachment: template_subsets.diff added

Done.

comment:4 Changed 4 years ago by leper

Keywords: review added

comment:5 Changed 4 years ago by sanderd17

This patch applies cleanly, but has some problems:

  • When trying to place a foundation, the following error is spit:
    ERROR: JavaScript error: gui/session/input.js line 112
    TypeError: result is undefined
      updateBuildingPlacementPreview@gui/session/input.js:112:36
      handleInputAfterGui@gui/session/input.js:1103:4
    ERROR: JavaScript error: simulation/components/GuiInterface.js line 1018
    TypeError: cmpOwnership is null
      GuiInterface.prototype.SetBuildingPlacementPreview@simulation/components/GuiInterface.js:1018:3
      GuiInterface.prototype.ScriptCall@simulation/components/GuiInterface.js:1914:3
      updateBuildingPlacementPreview@gui/session/input.js:108:5
      updateGUIObjects@gui/session/session.js:698:2
      onSimulationUpdate@gui/session/session.js:679:2
      __eventhandler162 (simulationupdate)@sn simulationupdate:0:1
    ERROR: Error calling component script function ScriptCall
    
  • When selecting a miraged tree (that was being gathered), I got the following error:
    WARNING: JavaScript warning: gui/session/selection_details.js line 235
    Script value conversion check failed: v.isString() || v.isNumber() (got type undefined)
    
  • And when one of my units died, I got this error:
    ERROR: Error in timer on entity 10080, IID 45, function MissileHit: TypeError: cmpCorpsePosition is null
      Health.prototype.CreateCorpse@simulation/components/Health.js:294:2
      Health.prototype.Reduce@simulation/components/Health.js:210:5
      Armour.prototype.TakeDamage@simulation/components/Armour.js:66:2
      Damage.CauseDamage@simulation/helpers/Damage.js:75:6
      Attack.prototype.MissileHit@simulation/components/Attack.js:702:3
      Timer.prototype.OnUpdate@simulation/components/Timer.js:100:4
    

I also tested killing and gathering a deer, this worked without problem. All these errors seem to be missing some copying of data or components from one template to the other.

I do fully agree with the way the patch solves this issue.

comment:6 Changed 4 years ago by sanderd17

Keywords: patch added; review removed
Milestone: BacklogAlpha 21
Summary: Dynamically generate templates for special entites[PATCH] Dynamically generate templates for special entites
Type: defectenhancement

comment:7 Changed 4 years ago by sanderd17

Found some sources of issues, but not sure if they're fixes.

  • Removing the "BuildRestrictions?" from corpse fixed the errors for killed units
  • Removing the "UnitMotion?" for previews fixed the foundation placing
  • The foundation element now complains about "Health" not having a "Max" defined
  • And I still don't understand the miraging warning

So AFAICS, there's something wrong with merging the components, as it shouldn't include components that aren't in the original template.

comment:8 Changed 4 years ago by fatherbushido

Trying to delete an unit, i got this error

ERROR: JavaScript error: simulation/components/Health.js line 294
TypeError: cmpCorpsePosition is null
  Health.prototype.CreateCorpse@simulation/components/Health.js:294:2
  Health.prototype.Reduce@simulation/components/Health.js:210:5
  Health.prototype.Kill@simulation/components/Health.js:171:2
  g_Commands["delete-entities"]@simulation/helpers/Commands.js:380:6
  ProcessCommand@simulation/helpers/Commands.js:45:3

(it's the same error as above)

Last edited 4 years ago by fatherbushido (previous) (diff)

Changed 4 years ago by leper

Attachment: template_subsets_fix.diff added

Incremental patch on top of the earlier one. Fixes issues found in the review.

comment:9 Changed 4 years ago by leper

Keywords: review added

There was indeed an issue with adding elements that shouldn't be added.

The incremental diff should be merged into the other one, and is just here to make reviewing the changes easier.

The new filtered if could possibly use the result of the find for moving, not sure about that. Apart from that refer to the commit message in the incremental diff.

comment:10 Changed 4 years ago by mimo

In 18042:

Add market component to foundations. This is needed since r18028 to be able to set
a market rallyPoint on a market foundation. refs #2951"

comment:11 Changed 4 years ago by mimo

In 18145:

add non-blocking obstruction to mirages, fixes #3957, refs #2951

comment:12 Changed 4 years ago by Itms

Keywords: rfc added; review removed

Move tickets from the review queue to the rfc one.

comment:13 Changed 4 years ago by elexis

I have created a git branch on https://github.com/elexis1/0ad/tree/2951-dynamic-templates where fixed 11 things that were annoying or broken with the path.

The patch was tested in about 8 svn matches without hash mismatch (besides the lighthouse foundation bug).

It could be committed, but perhaps someone can find solutions for two problems I couldn't solve yet:

1) corpse.xml and preview.xml share most of their code and a big comment. That should be deduplicated by using inheritance.

However that would come with the disadvantage that every property that one of the two files uses will also have to be merged in the inherited template.

2) It would be great if the "actor|" case would also be handled in the same manner. The only thing holding it back is the insertion of the template name to the template. It might be better to still have an XML file and reduce the hardcoded C++ part to a minimum (f.e. only the VisualActor? part).

Changed 4 years ago by elexis

Attachment: actor_wip.diff added

Broken, but shows the approach (not convinced it's preferable)

comment:14 Changed 4 years ago by elexis

Is there a better name for special_filter? Since it also adds things, perhaps something about transformations is more accurate?

comment:15 Changed 4 years ago by leper

Comments on changesets in order: Why do I even bother writing half a novel in a commit message when nobody bothers to actually apply it properly (man git-am)? Same. Splitting that cleanup out shouldn't be that hard, also now that we have a single line in the body of each if we should remove the braces. I had thought about moving that to a function of its own, but it didn't seem worthwhile; also using that much space seems pointless as error reporting will not make it any easier to find, nor does just doing it for that code make any sense. Why remove that TODO if we're not looking into fixing it? No, so we should just add test that will silently break because understanding that one file does not need to be updated at all is too hard? Meh. Ack. Ack. How is that any better, given that comments are unlikely to be read or updated (especially if they are at the other end of the file)? Meh (didn't modify anything that was in the original version to make the review easier). Ack (assuming someone verified that it does the same now). Ack (same as last one).

actor| is different and doesn't fit this ticket, as you've seen with that broken diff above.

So far nobody has come up with a better name and I slightly doubt that will change given that it has been half a year since I asked for better suggestions.

comment:16 Changed 4 years ago by elexis

  • Didn't know git-am yet, thx
  • I had to add the whitespace to the cpp files to make it readable in the first place and it should stay readable to other readers as well IMO
  • I'm not against the TODO, only against the copy of the TODO
  • Copying the comment to three places is not that nice IMO, people could just read the comment of the file they break
  • Braces -> removed
Last edited 4 years ago by elexis (previous) (diff)

Changed 4 years ago by elexis

merged github commits

Changed 4 years ago by elexis

Attachment: commands_r18651.txt added

sample

comment:17 Changed 4 years ago by elexis

In 18696:

Template manager cleanup. Patch by leper, refs #2951.

Move an early return to the top of the function.
Add an early return in case an error was determined.
Remove unneeded variables ok and templatePath.
Replace includeSubdirectories boolean with a flags int to merge duplicate vfs::ForEachFile? calls.

comment:18 Changed 4 years ago by elexis

Keywords: review added; rfc removed

IMO it is bad to have that test mod xml copy. Maybe the tests don't break if people don't update it, but everytime someone looks that one up and checks the related code, stumbling on the other duplicate, will notice that they differ, have to find out why they differ and if it's relevant. If the logic is changed, both copies have to be updated.

Don't see a reason why we would throw a warning if the public mod doesn't exist. If the tests don't exist, then they can't fail. If someone wants to distribute a custom mod without the public one, that should be possible without throwing warnings.

If the public mod doesn't exist, the "foundation|" transformation isn't available, but if we were to keep the copy, it would still test for it (sounds broken). It doesn't fail silently, it doesn't test for it.

In many places of source/ there is hardcoded logic which is only valid with public/. Perhaps that could be addressed at some point by moving the affected code, like this test.

Changed 4 years ago by elexis

Attachment: 2951_git_commits.7z added

Backup of the 14 git commits making up the committed cleanup and logic changing part for r18692.

comment:19 Changed 4 years ago by elexis

Keywords: rfc added; review removed

As suggested by Itms the tests could ideally become JS tests, so that changing the XML file doesn't mean that the C++ code has to be adapted (which was the purpose of this ticket I guess). That should also solve the mounting issue.

comment:20 Changed 4 years ago by elexis

Milestone: Alpha 21Alpha 22

Feature freeze in 2 days.

comment:21 Changed 4 years ago by elexis

Milestone: Alpha 22Work In Progress

Moving to the new WIP milestone.

comment:22 Changed 4 years ago by Itms

Owner: set to Itms

Since I asked for this feature I should be the one trying to merge this patch :P

comment:23 Changed 3 years ago by elexis

Keywords: rfc removed

Keeping the branch discussed above at https://github.com/elexis1/0ad/tree/2951-dynamic-templates

Forked that branch to https://github.com/elexis1/0ad/tree/2951_dynamic_templates_2017 Rebased that branch (without fast forwarding) and dropped some commits you didn't like:

drop 165ed25 Make public mod checks optional instead of copying files from the public to the test mod.
drop e2f9552 Make hardcoded XML in cpp files readable.
drop a9e9971 Merge the three duplications of the Actor Viewer comment per duplicate file. Thanks Itms for suggesting this 2016-08-06.
drop bb915d8 Remove braces.

Then rebased the branch after fast forwarding it. Solved the merge conflicts with your OOS paramNode (roman temple) fix from r18942 and my trailing whitespace removal from r18989 and added the 2017 brand.

Hence the new branch contains your two rebased diffs you have uploaded here and the commits in that branch that were not committed to svn already (r18696) and were accepted by you.

Differential Revision proposal at https://code.wildfiregames.com/D215

comment:24 Changed 3 years ago by elexis

Resolution: fixed
Status: newclosed

In 19302:

Use XML files instead of hardcoded C++ code in the template manager to universally change template properties at load time.

Patch By: leper
Differential Revision: https://code.wildfiregames.com/D215
Fixes #2951

comment:25 Changed 3 years ago by elexis

Milestone: Work In ProgressAlpha 22
Priority: Should HaveMust Have
Type: enhancementdefect

Thanks for the patch and enjoy the cake.

comment:26 Changed 3 years ago by leper

Apparently I did write that novel-length commit message for entertainment purposes only. As for the cake I guess I'll have to buy some ingredients and post pictures.

comment:27 Changed 3 years ago by elexis

Also a copy_from operator or something similar would be good for https://code.wildfiregames.com/D227?id=820#f7b76670

comment:28 Changed 3 years ago by leper

Possibly, but I still don't really see the point. And if we get that we could also consider using something like xslt (which I'd recommend against, as that is one of the things that are exploited in xml libraries a lot).

comment:29 Changed 3 years ago by fatherbushido

The explanation of how it works are the reward of the track game.

comment:30 Changed 3 years ago by elexis

My mistake for not having added this to the commit message:

This moves template additions to xml files.
Permitted components are now done via filtered and merge.
For adding new nodes while ignoring possible previous values use replace.

Some explanation on how to use the new filter templates:
 * Previously permitted component types should be replaced by
   <Foo merge=""/>, which does not add the <Foo> node if it wasn't present.
 * To actually remove the other components the encompassing node should
   specify filtered="", which will remove all nodes which are present in the
   original, but not in the filter.
 * Additions that should be performed while ignoring a possible original
   node should specify replace="".
 * To add something to an existing node (or add that node if it does not exist)
   just use <Foo>...</Foo>.
Last edited 3 years ago by elexis (previous) (diff)

comment:31 Changed 3 years ago by elexis

Description: modified (diff)

In r20162 committed by mimo:

Template loading from the AI

Patch by Sandarac

Reviewed By: mimo

Trac Tickets: #4611

Differential Revision: ​​https://code.wildfiregames.com/D639

comment:32 Changed 3 years ago by elexis

In r20573: undeletable special filter template.

comment:33 Changed 2 years ago by elexis

In 21621:

Remove test templates that became unused with rP16033, refs #2951.

Note: See TracTickets for help on using tickets.