Opened 17 years ago

Closed 11 years ago

#3 closed enhancement (fixed)

[PATCH] Implement Technologies

Reported by: Stuart Walpole Owned by: Jonathan Waller
Priority: Must Have Milestone: Alpha 10
Component: Core engine Keywords: patch
Cc: Patch:

Description (last modified by Jonathan Waller)

Most of the code for technologies is now done.

The modifications that technologies make still need coding. See ResourceGatherer.js for an example.

Tech pairs where only one out of two techs can be researched need implementing. An idea is to have a tech pair json file which defines the pair and have the building have that in it's template file for what is available for research. The tech pairs can then supersede each other.

Attachments (5)

technology_20-03-12.2.diff (116.1 KB ) - added by Jonathan Waller 11 years ago.
proper patch now that svn is fixed
technologies.zip (85.8 KB ) - added by Jonathan Waller 11 years ago.
temporary icons to go in art/textures/ui/session/portraits
technology_07-04-12.2.diff (139.3 KB ) - added by Jonathan Waller 11 years ago.
technology_16-04-12.diff (139.5 KB ) - added by Jonathan Waller 11 years ago.
technology_19-04-12.diff (156.3 KB ) - added by Jonathan Waller 11 years ago.

Download all attachments as: .zip

Change History (38)

comment:1 by Stuart Walpole, 17 years ago

Description: modified (diff)

comment:2 by Matei, 17 years ago

Owner: set to Matei, Andrew

comment:3 by Matei, 16 years ago

Description: modified (diff)
Resolution: fixed
Status: newclosed

comment:4 by Jan Wassenberg, 15 years ago

Description: modified (diff)

comment:5 by (none), 12 years ago

Milestone: Playability Demo

Milestone Playability Demo deleted

comment:6 by michael, 11 years ago

Resolution: fixed
Status: closedreopened

comment:7 by historic_bruno, 11 years ago

Description: modified (diff)
Milestone: Backlog
Owner: Matei, Andrew removed
Status: reopenednew

comment:8 by historic_bruno, 11 years ago

Priority: Nice to HaveMust Have

comment:9 by Darth_Malloc, 11 years ago

Owner: set to Darth_Malloc
Status: newassigned

in reply to:  9 comment:10 by historic_bruno, 11 years ago

Replying to Darth_Malloc:

Hello Darth_Malloc, thanks for desiring to contribute to 0 A.D. :)

Technologies are a fairly major task and essential to the game. Can you let the developers know your plans and keep us updated on your progress? I'd suggest starting with a post in the Development & Technical Discussion section of the forum and possibly visiting us on IRC (QuakeNet #0ad-dev) as well.

comment:11 by historic_bruno, 11 years ago

Any progress?

comment:12 by historic_bruno, 11 years ago

Owner: Darth_Malloc removed
Status: assignednew

comment:13 by Darth_Malloc, 11 years ago

Owner: set to Darth_Malloc

in reply to:  13 ; comment:14 by Jonathan Waller, 11 years ago

Replying to Darth_Malloc: Hi,

I have just started looking into making technologies work, though I neglected to mark this on the ticket. If you have made progress please post it here so that the games developers can take a look.

If you don't post further within 24 hours I am afraid that (based on past experience) I will have to assume that you are not progressing with this and so will reassign the ticket and continue with my implementation.

in reply to:  14 comment:15 by Darth_Malloc, 11 years ago

Replying to quantumstate:

Replying to Darth_Malloc: Hi,

I have just started looking into making technologies work, though I neglected to mark this on the ticket. If you have made progress please post it here so that the games developers can take a look.

If you don't post further within 24 hours I am afraid that (based on past experience) I will have to assume that you are not progressing with this and so will reassign the ticket and continue with my implementation.

Hi quantumstate,

I will be getting to the technologies soon, however, I would say that there are many other tickets that take higher priority, as it would be better to add other bug fixes and optimizations before implementing a new feature. In any case, the list of techs is not in a completed state - Only the Hellene techs are listed, so it might be better to wait until all civs have completed tech lists, so as to prevent balancing issues. The task of techs is not so much complicated as time-consuming, and I have a lot of school work right now, so my spare time is limited. That said, I am curious about your proposed implementation, and whether it is similar to my own idea. I already have a fairly good idea of how I am going to implement the techs, it is just a matter of having the time to write that much code.

comment:16 by Jonathan Waller, 11 years ago

Owner: changed from Darth_Malloc to Jonathan Waller

Hi,

The developer team consider this to be the highest priority new feature since it will have massive gameplay consequences and will touch lots of areas. Since you say you are fairly busy and have other tickets as priority, I have reassigned this to myself. Initially I will implement the 'unlocking' techs since the other techs are simply features added to this.

The rough plan is to add a new component to the player, called TechnologyManager which will load in the data files and keep track of what has been researched. Then I am modifying the TrainingQueue to be a generic ProductionQueue component, this will handle researching techs. Units and buildings will also have a component modified/added so the unlocking requirements can be added.

So far I have a basic TechnologyManager implemented and have started modifying TraingingQueue.

After some discussion the initial format for the techs is this json:

{
	"town_phase": {
		"name": "Town Phase",
		"description": "Advances from a small village to an bustling town, ready to expand rapidly.",
		"cost": {"food": 200, "wood": 200},
		"requires": "Sewers",
		"supercedes": "Village Phase",
		"icon": "town_phase.png",
		"researchTime": 100
	}
	};

I would be interested in hearing your thoughts about the implementation.

in reply to:  16 ; comment:17 by Darth_Malloc, 11 years ago

Replying to quantumstate:

Hi,

The developer team consider this to be the highest priority new feature since it will have massive gameplay consequences and will touch lots of areas. Since you say you are fairly busy and have other tickets as priority, I have reassigned this to myself. Initially I will implement the 'unlocking' techs since the other techs are simply features added to this.

The rough plan is to add a new component to the player, called TechnologyManager which will load in the data files and keep track of what has been researched. Then I am modifying the TrainingQueue to be a generic ProductionQueue component, this will handle researching techs. Units and buildings will also have a component modified/added so the unlocking requirements can be added.

So far I have a basic TechnologyManager implemented and have started modifying TraingingQueue.

After some discussion the initial format for the techs is this json:

{
	"town_phase": {
		"name": "Town Phase",
		"description": "Advances from a small village to an bustling town, ready to expand rapidly.",
		"cost": {"food": 200, "wood": 200},
		"requires": "Sewers",
		"supercedes": "Village Phase",
		"icon": "town_phase.png",
		"researchTime": 100
	}
	};

I would be interested in hearing your thoughts about the implementation.

Hi,

I like this implementation. It is fairly similar to my own idea. The algorithm that I had thought of was pretty much the same, except that I had not gotten around to the code itself yet. The truth is, I was really hoping to contribute to this feature to some extent. Since it is a lot of code to modify, I was wondering if it would be alright for us to work together on this one.

in reply to:  17 comment:18 by Jonathan Waller, 11 years ago

Replying to Darth_Malloc:

I like this implementation. It is fairly similar to my own idea. The algorithm that I had thought of was pretty much the same, except that I had not gotten around to the code itself yet. The truth is, I was really hoping to contribute to this feature to some extent. Since it is a lot of code to modify, I was wondering if it would be alright for us to work together on this one.

As you say there is quite a lot of code to modify to get the whole patch working. The initial part with just unlocking techs being available is basically all in one place though. I can't see an easy way to split that bit of the task into multiple parts, especially with communication over the internet. If you look at previous development work there is only one person working on a component at a time.

My main concern is that in the past you have vanished for months and the only code I can find by you is a single header file with the Atlas crash bug. This makes me reluctant to work with you because if you are writing some code that I am relying on it is very annoying if I wait wondering if it will ever be written, and I have no idea how well you can code.

I am using git to develop this so I can put the repository on github. I am not currently willing to work together with you on this code for the reasons above, at least until the basic technology implementation is finished, sorry.

comment:19 by Erik Johansson, 11 years ago

to Darth_Malloc: In other words: Please focus on the other tickets for now, and participating in the discussions in the forums/IRC, that way you will be trusted as well as be more familiar with both the code and the development/community.

Last edited 11 years ago by Erik Johansson (previous) (diff)

comment:20 by Jonathan Waller, 11 years ago

I have attached my patch. I think using sed to do a replace on the technologies folder has made svn unhappy so I have attached the whole compressed folder. Renaming the trainingManager module has touched a lot of code with name changes which has made the patch bulkier.

by Jonathan Waller, 11 years ago

Attachment: technology_20-03-12.2.diff added

proper patch now that svn is fixed

by Jonathan Waller, 11 years ago

Attachment: technologies.zip added

temporary icons to go in art/textures/ui/session/portraits

comment:21 by Jonathan Waller, 11 years ago

I have attached a fixed version of the diff.

comment:22 by Jonathan Waller, 11 years ago

Keywords: review added
Summary: Implement Technologies[PATCH] Implement Technologies

comment:23 by Kieran P, 11 years ago

Keywords: patch added
Milestone: BacklogAlpha 10
Type: taskenhancement

comment:24 by Jonathan Waller, 11 years ago

I have attached a new patch with modifying technologies implemented. For each value which needs to be modified there will have to be some code changes, currently only gathering rates are working. This is a straightforward process however.

Edit: The technology requirements are currently pretty limited as well. They are self cotained so can be implemented as needed.

To apply the patch you will need to copy TrainingQueue.js to ProductionQueue.js in both the components and the components/interfaces directories. The file rename seems to have confused the patch generating tool.

Last edited 11 years ago by Jonathan Waller (previous) (diff)

by Jonathan Waller, 11 years ago

Attachment: technology_07-04-12.2.diff added

comment:25 by leper, 11 years ago

Your last patch has the json files twice.

I looked at both patches and I have some suggestions. I hope I haven't missed anything as the patch is huge :-).

Not related to your patch but we should think about it: How about adding alternating technologies? Maybe more than one tech that can supercede another, but only one can be researched during a game?

Regarding the patch: We should allow a unit/building/tech to have multiple requirements. That would affect unit_commands.js around line 421, GuiInterface.js where we should replace ret.requiredTechnology with ret.requiredTechnologies and Identity.js using requiredTechnologies.

Now some suggestions grouped by file.

unit_commands.js
line 226: The added indentation is unnecessary.
lines 211-244: That block needs restructuring. We should probably replace it with a switch(guiName) and use somthing like

case FORMATION:
case COMMAND:
case STANCE:
   break;

to replace that one if.
line 643: The comment isn't really needed. I would recommend to remove the similar lines in that function too.

GuiInterface.js
line 357: You should add a warning to if (!template) as indicated by the comment in unit_commands.js

TechnologyManager.js
rename allTech, researchedTech and inProgressTech to allTechs, researchedTechs and inProgressTechs as that can contain more than one technology.
line 30: Typos: "fnuction" -> "function" and "then" -> "them"
line 77: ret = ret && ... is not needed. ret = ...; should work too.
lines 84-107: Shouldn't be indented as _UpdateAutoResearch is not indented either. We should probably use a for each loop instead of that for loop.
line 136: The comment misses an
line 165: Move this._UpdateAutoResearch(); to the end of the if block (line 159)
ApplyModifications(): We should add profiler commands to measure the performance impact to decide if we need caching.
lines 203-208: Add a warning to the if-else-if block (as a final else) to notify modders that we can't find an appropriate modification.

ProductionQueue.js
lines 12-24: Add the <optional> tags on their own line and indent the tags enclosed by them.
line 48: Update the comment as we have a tech and not a unit (also techs can only be researched once and therefore not be batched).
lines 112-113: I would add some braces
line 207: We should add a warning as entering that else block indicates a typo or a failed modding attempt.

ScriptInterface.cpp:
The comments in FindAllTechnologyTemplates() probably needs to be updated as it doesn't really apply to technologies.
Should we rename UnBlockTraining to UnBlockProduction?

That's it. Overall a nice patch.

in reply to:  25 comment:26 by Jonathan Waller, 11 years ago

Thanks for the review, lots of helpful comments. I have some comments in reply for some of them, the rest i went ahead and did, the new patch is attached.

Replying to leper:

Not related to your patch but we should think about it: How about adding alternating technologies? Maybe more than one tech that can supercede another, but only one can be researched during a game?

I thought about this but it gets fairly complicated since you then might want things superceding the alternating techs so you get merging as well as splitting to tech branches. We were planning to do this for phases I think but that might not happen now. I don't think anything can be dnoe at this point to make it easier to add later so I will ignore this unless it is requested.

Regarding the patch: We should allow a unit/building/tech to have multiple requirements. That would affect unit_commands.js around line 421, GuiInterface.js where we should replace ret.requiredTechnology with ret.requiredTechnologies and Identity.js using requiredTechnologies.

There was some discussion about this and from what I remember it was decided that requiring multiple technologies would be making things too complicated for the user. Technologies themselves can have mroe complex requirements already.

TechnologyManager.js
line 77: ret = ret && ... is not needed. ret = ...; should work too.

Although the current code is not strictly necessary as you say I think the existing version conveys the intent better.

lines 84-107: We should probably use a for each loop instead of that for loop.

I am not a big fan of for each because it is non standard

ApplyModifications(): We should add profiler commands to measure the performance impact to decide if we need caching.

I actually decided we needed caching and it is implemented on the component side, this is what the TechnologyModificationChange message is helping with. I have removed the comment since it is out of date.

Should we rename UnBlockTraining to UnBlockProduction?

No, I deliberately kept the old name since this will not block research since it is population limited.

by Jonathan Waller, 11 years ago

Attachment: technology_16-04-12.diff added

comment:27 by leper, 11 years ago

I thought about your comments and all of them seem reasonable.

Some more things I found in your last patch (or that occured to me while reviewing it and reading some discussions):

We want different tech trees for civilizations. They will consist of a subset of all techs, but some wont be available to a specific civ or some modifications may be different.

We could create {civ}/ subfolders and add jsons there that reference the tech they are based on. Something like this could work

{
    "specificName": "",
    "base": "plough",
    "description": "",
    "affects": ["Female", "Citizen Soldier"]
}

Where only specificName and base are needed, the rest is optional. All techs available to a civ would be in the civs subfolder. This would probably require some modification to the c++ code, TechnologyTemplateManager, TechnologyManager and ProductionQueue. The code would need to loop over the "base" tech first and then replace all values that are defined in the civ specific tech json.

We should implement something like this, but we could also duplicate the techs for each civ but that would make changes more difficult. (This is just a suggestion and if you can think of a better way to achieve different tech trees or you see some issues with this suggestion please say so.)

You should probably delete SetPhase and GetPhase in Player.js as the same can be achieved by querying TechnologyManager.

unit_commands.js: Replace the TODO comment with something similar to the other comments as GetTechnologyData already warns the user if we have no valid template.

TechnologyManager.js
Add a warning to if (!template) in CanResearch.
ApplyModifications (and tech jsons): Should we add a modification.subtract as using modification.add with a negative value is slightly awkward. (Mythos_Ruler wants to make techs that add eg. armour and decrease speed)

template_structure_civic_civil_centre.xml has some indentation with tabs and spaces. You should use spaces in the xml files.

by Jonathan Waller, 11 years ago

Attachment: technology_19-04-12.diff added

comment:28 by Philip Taylor, 11 years ago

Haven't tried to actually run this (I assume it'll work fine), and the general design looks sane, so just some minor details from reading the patch:

ComponentManager.cpp

  • I think CComponentManager::Script_FindAllTechnologyTemplates() should be CComponentManager::Script_FindJSONFiles("technologies"), taking a path relative to simulation/data/ (to match the filenames accepted by Script_ReadJSONFile), because that genericity shouldn't add significant complexity (other than maybe creating a struct to pass as cbData) and the engine should try to be reasonably gameplay-agnostic when possible, and it seems quite possible we'll eventually add other scripts that want to find other JSON files.

CSimulation2.cpp

  • The LOAD_SCRIPTED_COMPONENT lines should be kept alphabetical.

input.js

// Called by GUI when user clicks training button 
function addResearchToQueue(entity, trainEntType) 

Presumably shouldn't be "training button", nor "trainEntType", since this isn't training.

GuiInterface.js

  • Function assignments to prototype properties should end with semicolons.
  • "invaalid"
  • "template["specificName_" + cmpPlayer.GetCiv()]" - seems it should be cleaner to do something like "template.specificName[cmpPlayer.GetCiv()]" and have the JSON files say
      "specificName": {
        "generic": "Generic Specific City Name", 
        "hele": "Greek City"
      }
    
    rather than trying to indirectly encode the civ-to-name mapping in property suffixes.

ResourceGatherer.js

  • Does the cache need to get invalidated by an OnOwnershipChanged, to avoid stale data when a unit gets captured?
  • GetGatherRates - the "{" should be on its own line.
  • Inconsistent use of cmpTechManager here, vs cmpTechMan in other scripts. Should be consistent about it one way or the other.

test_GuiInterface.js

  • LoadComponentScripts lines should be kept alphabetical.

TechnologyManager.js

  • The "Example data" example could be formatted much more readably by splitting lines and indenting sensibly.
  • "TechnologyManager.prototype._UpdateAutoResearch" - underscore-prefixing for private methods isn't a convention that we use, so it's better not to start doing that here. (It could be argued that we ought to have that convention, but we should decide that first and then do it consistently.)
  • "This function checks an entity template to see if it's technology requirements have been met" - should be "its". Also, saying "This function" seems redundant (and inconsistent with CanResearch etc).
  • CanResearch: The logic would probably be clearer by doing "if (template.supercedes && !this.IsTechnologyResearched(template.supercedes)) return false;" etc, instead of accumulating in the ret variable.
  • "supercedes" (lots of places) - that spelling seems to be widely regarded as erroneous - should be "supersedes".
  • _CheckTechnologyRequirements: Similar comment about doing an immediate "return false;" instead of &&ing everything together. (Also that would match the reqs.any case much more closely.)
  • _CheckTechnologyRequirements: Missing semicolon on "return false" in the reqs.any case.
  • Use reqs.class, not reqs["class"]
  • ResearchTechnology: I think it's nicer to do
    if (!template)
    {
        error("...");
        return;
    }
    ...
    
    because then the error message is close to the error condition, and you don't have to indent the rest of the function body by an extra level.
  • "template.affects[i].split(" ")" - probably should be split(/\s+/) to match the usual splitting of token strings.
  • "We add an item to this.modifications for every" - for every what?
  • "mod[j] = modification[j]" - missing semicolon.
  • "orignal"
  • ApplyModifications: applies is computed but never used for anything.

ProductionQueue.js

  • a:help text for <Technologies> shouldn't end in a "."
  • "superceded"
  • "Also store the "children" for the superceding," - looks like an unfinished
  • In ProgressTimeout, you need to do "time -= item.timeRemaining;" when shifting the tech off the queue (as with unit training). Otherwise the time that's notionally spent finishing this production will be subtracted from the next item in the queue too.

plough.json

  • "sod" - should be "sod."
  • "technologies/city_phase.png" - presumably all tech icons will be in that directory (to avoid chaotic organisation), so it might be better to just say "city_phase.png" and get the GUI to prepend the "technologies/".

comment:29 by historic_bruno, 11 years ago

Simple question: what unit is researchTime? It might be seconds, milliseconds, or game turns :) Could we name that field according to its units, since we lose the ability to annotate these things in JSON?

comment:30 by Jonathan Waller, 11 years ago

In 11584:

Technologies. Refs #3. Full unlocking technology implementation. Only unit gathering rates can be modified currently because the patch was big enough already.

comment:31 by Kieran P, 11 years ago

Looking good. Some quick notes:

  • The way the code needs to be changed to apply updates to units seems a bit extreme. Rather than apply techs to entities when they're needed (gathering rates, armor, etc), can't we make it so this.entities already returns the upgraded units? Seems better that way, so people don't need to worry about upgrading the units before doing anything (the unit AI is already large enough). Maybe a EntityCollection (unrelated to AI) and do this.entities = new EntityCollection(this.entities) where EntityCollection would apply all relevant techs to the units. You can probably think of something cleaner. The commits for hacing in techs though seem awfully ugly. Surely there is a cleaner way?
  • The files in mods/public/technologies can probably go now
  • I can see a tech modifying different units in different ways. So I think "affects" needs to be moved out of the root tech json and into the json maps/hashes for modifications. e.g.
"modifications": [
  {
    "value": "ResourceGatherer/Rates/food.grain",
    "multiplier": 15
  }
],
"affects": ["Female"]

should be

"modifications": [
  {
    "value": "ResourceGatherer/Rates/food.grain",
    "multiplier": 15,
    "affects": ["Female"]
  }
]
  • I can see techs needing multiple requirements, so it would be nice for it to act like modifications and use an array instead.
"requirements": [
  {
    "class": "Barracks",
    "numberOfTypes": 1
  },
  {
    "class": "Mill",
    "numberOfTypes": 2
  }
]

in reply to:  31 comment:32 by Jonathan Waller, 11 years ago

Description: modified (diff)

@historic_bruno I have added documentation to the wiki at wiki:Technology_Templates which specifies the units. Adding it to the property name seems quite ugly.

@k776 Thanks for the feedback, I have added the individual modification affects basically as you said. The old affects is still there in addition as discussed on irc.

I have also modified the caching code to be done in TechnologyManager so the code is significantly cleaner in the other components.

I don't know if the old technologies folder is still wanted for reference?

comment:33 by historic_bruno, 11 years ago

Keywords: review removed
Resolution: fixed
Status: newclosed

Let's create new tickets as needed for issues related to techs.

Note: See TracTickets for help on using tickets.