Min and Max Distance can be modified by auras/technologies.
Details
Details
Fixed some variables.
While the code works with a preview entity, distance used was the one related to the template "{civ}_fortress" without process any change made to the template distance.
Knowing right the template name, getting knowledge of its template and using the funcitionApplyModificationsToTemplate() is possible.
Diff Detail
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
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Before this gets committed, it should also be tested with a tech and maybe a ranged and player aura. Expecting it to work, but verifying is better.
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
|---|---|---|
| 258 ↗ | (On Diff #1002) | Only one check in a simulation helper that was needed for tests. That check should likely be removed even and the template manager become mocked. This specific system is not likely to not exist |
| 260 ↗ | (On Diff #1002) | I was wondering whether templateName.substr(templateName.lastIndexOf("|") + 1) was good there to get rid of "preview|" in the most agnostic way (perhaps someone would add some special filters for any reason sometime), |
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
|---|---|---|
| 258 ↗ | (On Diff #1006) | Hmm, something strange, if do a check then it should be like: let cmpTemplateManager = Engine.QueryInterface(SYSTEM_ENTITY, IID_TemplateManager);
if (!cmpTemplateManager)
return; |
Comment Actions
fatherbushido raised some valid points in irc that shoud be tested:
- If someone would add a technology changing the requirements, does it still work (or will people be forced to revise this patch if they tried to add a technology with that effect?)
- Is the AI aware of this change? Grep through the simulation/ai/ code for MinDistance and someone should check whether it receives the updated value
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
|---|---|---|
| 258 ↗ | (On Diff #1006) | Agree with Vladislav, the if statement is always true and if we assume that the template manager system component doesn't exist, it will still error out when trying to access it below. Should we really add the check though? I don't assume the template manager can be removed from the game. |
| 262 ↗ | (On Diff #1006) | If was wondering if this is the best way to get rid of the "preview|" prefix or whether we should throw an error if !templateName.startsWith("preview|"). |
| 282 ↗ | (On Diff #1006) | one line with let, one with var? |
Comment Actions
modified some variables from var to let
better management of the cmpTemplateManager check.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/656/ for more details.
Comment Actions
Thanks for the update, code looks much saner.
Did you test it with a sample tech? (Basically I can't imagine it to break, but why not test for completeness).
Also did you search for the AI code? (Just grep for MinDistance everywhere in simulation/ and globalscripts/).
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
|---|---|---|
| 281 ↗ | (On Diff #1028) | Not really an issue, but perhaps naming it minDistance would be a bit nicer |
| 282 ↗ | (On Diff #1028) | nearEnts does not contain a list of entities anymore, correct? But a boolean true / false, so perhaps the name should be changed. |
| 258 ↗ | (On Diff #1006) | Question wasn't answered here yet. Or can I take your updated diff as an answer that we need this check for the template manager existEnce? It seems we have decided in rP19363 to remove all other (= 1) of these checks |
| 260 ↗ | (On Diff #1002) | That's basically a question for someone who is familiar with the code ;-) |
Comment Actions
That what fatherbushido said, and AI support also doesn't seem given:
In entity.js we have
buildDistance: function() {
return this.get("BuildRestrictions/Distance");
},And get accounts for tech modifications (so aura must be in there too)
// helper function to return a template value, optionally adjusting for tech.
But since get isn't recursive, we must call get on each property, similar to the "cost" function in that file.
Comment Actions
Removed the nearEnt variable, using directly the cmpRangeManager variable to check the boolean result.
minDist and maxDist replaced the previous temprorary variable dist for easier reading and more clear distinction.
After some tests, the AI can't recognize the modify of the minDistance or maxDistance probably because the buildRestriction function in the entity.js component has preview template information instead of the real template name.
Comment Actions
Are you sure about this or is this a guess? I doubt that the AI uses the preview template which is mean for visual indication.
You should also find someone who accepts the design of the alexander aura. I think we at least agree on the idea of being able to change the build restrictions with an aura or tech, if not for 0ad, then for a mod thereof.
programming.json entry missing again.
Comment Actions
no I am not sure, it is a guess. Looks like the AI doesn't take count of the modifications to template though, or, at least, i used an aura and modified the minDistance of a building and the values displayed were correct.
Despite the train of the Hero able to change the minDistance, the ai didn't show the new values using a warn( ) function. I guess it doesn't work for the AI.
You should also find someone who accepts the design of the alexander aura. I think we at least agree on the idea of being able to change the build restrictions with an aura or tech, if not for 0ad, then for a mod thereof.
This build restriction change is required for Alexander hero aura and for an eventual iberian Caros hero aura introduction, but i doubt that this patch will be approved since AI doesn't correctly recognize the change. Revealing in advice this this supposition, the D231 is changed aswell.
Comment Actions
A more complete diff
by Grugnas. We were suspecting that the Aura only affects entities while we need a template modification, but then again the athen hero pericles also modifies build costs of temples. At some point we might want to ask some AI developer, but I think we should do some actual debugging beforehand.Comment Actions
And also answer the first question I asked on #0ad-dev, do you want use it for a range aura? :)
Comment Actions
The minimum distance aura is a global aura because it is going to affect a specific player structure identity generic name independently from the hero current position.
Comment Actions
I hope this is enough to let Template.js have info about evntual distance restriction modify.
Comment Actions
Build is green
Updating workspaces. Build (release)... Build (debug)... Running release tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK! Running debug tests... Running cxxtest tests (305 tests).................................................................................................................................................................................................................................................................................................................OK!
http://jw:8080/job/phabricator/787/ for more details.
Comment Actions
I believe that changing templates.js is enough to let the ai know of the change, however testing with setting one and warning the value out in mapModule.js doesn't change the value. Tracking the bug further down reveals that the modification is passed correctly to AIInterface.js and shared.js. I guess the values are not correctly transferred to _templateModif in entity.js (for entity's they are updated at least in init for templates seems nowhere). I guess this bug is bigger than just this patch thus adding @mimo for this issue.
Add this to the patch for using the modifications, by running an ai match notice that when the ai builds a tower, 60 is warned out the whole match, instead of 50 after city phase:
Index: binaries/data/mods/public/simulation/data/technologies/phase_city.json =================================================================== --- binaries/data/mods/public/simulation/data/technologies/phase_city.json (revision 19950) +++ binaries/data/mods/public/simulation/data/technologies/phase_city.json (working copy) @@ -15,7 +15,8 @@ "modifications": [ { "value": "TerritoryInfluence/Radius", "multiply": 1.50, "affects": "CivCentre" }, { "value": "Health/Max", "multiply": 1.1, "affects": "CitizenSoldier" }, - { "value": "Capturable/GarrisonRegenRate", "add": 9.0, "affects": "Structure" } + { "value": "Capturable/GarrisonRegenRate", "add": 9.0, "affects": "Structure" }, + {"value": "BuildRestrictions/Distance/MinDistance", "add": -10, "affects": "Structure"} ], "soundComplete": "interface/alarm/alarm_phase.xml" }Index: binaries/data/mods/public/simulation/ai/petra/mapModule.js =================================================================== --- binaries/data/mods/public/simulation/ai/petra/mapModule.js (revision 19950) +++ binaries/data/mods/public/simulation/ai/petra/mapModule.js (working copy) @@ -88,6 +88,7 @@ if (template && template.buildDistance()) { let minDist = +template.buildDistance().MinDistance; + warn(minDist); let fromClass = template.buildDistance().FromClass; if (minDist && fromClass) {
| binaries/data/mods/public/globalscripts/Templates.js | ||
|---|---|---|
| 219–222 ↗ | (On Diff #1282) | BuildRestrictions *2 (testing gives the i's doesn't matter for the AI) |
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
| 259–260 ↗ | (On Diff #1282) | no need for checking system components |
| 262–263 ↗ | (On Diff #1282) | second assignment of templateName can be inlined |
| 278 ↗ | (On Diff #1282) | the template can contain a value of 0 and then a modification might change that, so I guess we need to check for !== undefined |
| 282 ↗ | (On Diff #1282) | inline (also an entity can have both min and max, so overwriting cmpRangeManager is wrong as it breaks that) |
| 304 ↗ | (On Diff #1282) | inline |
| binaries/data/mods/public/simulation/components/BuildRestrictions.js | ||
|---|---|---|
| 282 ↗ | (On Diff #1282) |
It seems that the "... " were not read by the patch uploader ;-) |
Comment Actions
Just having a quick look at the ticket, what's the status of the AI. You should add some changes in petra/mapModule.js
either instead of using "let minDist = +template.buildDistance().MinDistance;" you add a new function in common-api/entity.js which would return this.get("BuildRestrictions/Distance/MinDistance") and thus call +template.MinDistance()
or you call the get directly there.
i've not tested both solutions, but that should work: we have to pass through the get for the aura+tech to be applied.
Comment Actions
Why did you add that?
| binaries/data/mods/public/globalscripts/Templates.js | ||
|---|---|---|
| 232 ↗ | (On Diff #3187) | Those BuildRestr(i)ctions seem really hard to modified ;-) |
Comment Actions
Thanks for noticing the problem. Can you try with D811 (removing your current changes in entity.js)?