#2951 closed defect (fixed)
[PATCH] Dynamically generate templates for special entites — at Version 31
Reported by: | Itms | Owned by: | Itms |
---|---|---|---|
Priority: | Must Have | Milestone: | Alpha 22 |
Component: | Core engine | Keywords: | patch |
Cc: | Patch: |
Description (last modified by )
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.
Change History (38)
comment:1 by , 9 years ago
comment:3 by , 8 years ago
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
comment:4 by , 8 years ago
Keywords: | review added |
---|
comment:5 by , 8 years ago
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 by , 8 years ago
Keywords: | patch added; review removed |
---|---|
Milestone: | Backlog → Alpha 21 |
Summary: | Dynamically generate templates for special entites → [PATCH] Dynamically generate templates for special entites |
Type: | defect → enhancement |
comment:7 by , 8 years ago
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 by , 8 years ago
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)
by , 8 years ago
Attachment: | template_subsets_fix.diff added |
---|
Incremental patch on top of the earlier one. Fixes issues found in the review.
comment:9 by , 8 years ago
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:12 by , 8 years ago
Keywords: | rfc added; review removed |
---|
Move tickets from the review queue to the rfc one.
comment:13 by , 8 years ago
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).
by , 8 years ago
Attachment: | actor_wip.diff added |
---|
Broken, but shows the approach (not convinced it's preferable)
comment:14 by , 8 years ago
Is there a better name for special_filter
? Since it also adds things, perhaps something about transformations is more accurate?
comment:15 by , 8 years ago
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 by , 8 years ago
- 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
- Didn't get what you mean with
so we should just add test that will silently break because understanding that one file does not need to be updated at all
. You mean https://github.com/elexis1/0ad/commit/4a2b9dba0a5510b7a84df0a9b775b5b0c0ccce3b#diff-8ae41a42816a94f0e10f5e36fcf97da0 ?
comment:18 by , 8 years ago
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.
by , 8 years ago
Attachment: | 2951_git_commits.7z added |
---|
Backup of the 14 git commits making up the committed cleanup and logic changing part for r18692.
by , 8 years ago
Attachment: | 2951_template_subsets_v2_without_committed_cleanup.diff added |
---|
comment:19 by , 8 years ago
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:22 by , 7 years ago
Owner: | set to |
---|
Since I asked for this feature I should be the one trying to merge this patch :P
comment:23 by , 7 years ago
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:25 by , 7 years ago
Milestone: | Work In Progress → Alpha 22 |
---|---|
Priority: | Should Have → Must Have |
Type: | enhancement → defect |
Thanks for the patch and enjoy the cake.
comment:26 by , 7 years ago
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 by , 7 years ago
Also a copy_from operator or something similar would be good for https://code.wildfiregames.com/D227?id=820#f7b76670
comment:28 by , 7 years ago
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:30 by , 7 years ago
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>.
comment:31 by , 7 years ago
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
In 16033: