Answer TechnologyExists from memory instead of disk
ClosedPublic
Actions

Authored by elexis on Dec 4 2017, 3:00 AM.

Details

Reviewers

mimo
bb

Commits

rP20587: Check for Technology existence by looking up loaded technologies rather than…

Summary

Following rP20551, each time a building with a civ-specific production queue item is selected, the DataTemplateManager is asked if that technology exists.
It checks if the file exists on the disk, but it has already loaded all techs at the gamestart and
to me it sounds like it should always do that, because the files are small and we don't want to wait for the harddisk to get back from standby mode in the middle of a game.

The FileExists function is still useful, even if not called currently in vanilla.

Test Plan

amirite?

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

elexis created this revision.Dec 4 2017, 3:00 AM

Harbormaster completed remote builds in B3941: Diff 4521.Dec 4 2017, 3:00 AM

Executing section Default...
Executing section Source...
Executing section JS...

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 201| »   for·(var·i·in·techList)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 203| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 215| »   for·(var·i·=·0;·i·<·techList.length;·i++)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 217| »   »   var·tech·=·techList[i];
|    | [NORMAL] JSHintBear:
|    | 'tech' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 224| »   »   var·template·=·cmpTechnologyManager.GetTechnologyTemplate(tech);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 253| »   »   »   ||·cmpTechnologyManager.IsTechnologyResearched(template.bottom)·||·cmpTechnologyManager.IsInProgress(template.bottom));
|    | [NORMAL] JSHintBear:
|    | Misleading line break before '||'; readers may interpret this as an expression boundary.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 349| »   »   »   var·template·=·cmpDataTemplateManager.GetTechnologyTemplate(templateName);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 393| »   »   »   var·cmpTrigger·=·Engine.QueryInterface(SYSTEM_ENTITY,·IID_Trigger);
|    | [NORMAL] JSHintBear:
|    | 'cmpTrigger' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 432| »   for·(var·i·=·0;·i·<·this.queue.length;·++i)
|    | [NORMAL] JSHintBear:
|    | 'i' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 798| »   »   »   »   »   var·cmpPlayer·=·QueryOwnerInterface(this.entity);
|    | [NORMAL] JSHintBear:
|    | 'cmpPlayer' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 809| »   »   »   var·cmpTechnologyManager·=·QueryOwnerInterface(this.entity,·IID_TechnologyManager);
|    | [NORMAL] JSHintBear:
|    | 'cmpTechnologyManager' is already defined.

binaries/data/mods/public/simulation/components/ProductionQueue.js
| 812| »   »   »   var·template·=·cmpTechnologyManager.GetTechnologyTemplate(item.technologyTemplate);
|    | [NORMAL] JSHintBear:
|    | 'template' is already defined.

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

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (308 tests)....................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...

Harbormaster completed remote builds in B3941: Diff 4521.Dec 4 2017, 3:20 AM

I agree with the approach, but isn't there some inconsistency (not due to this patch but would be worth fixing) as it seems we allow "hotloading" of new techs without allowing it.

TechnologyExists knows only the templates loaded at init
GetTechnologyTemplate is written to be able to reload a tech which didn't existed at init, but would fail in that case because of its deepfreeze

We should decide if we allow it or not (i.e. removing or not the deepfreeze on this.allTechs, and freezing each individual tech if removed), and make TechnologyExists and GetTechnologyTemplate consistent with that choice (same comments applies for auras).

GetAllTechs would also have to load from disk instead of memory.

I was confused by the word hotloading, because we have specific hotloading code in C++, functional for the gui/ and apparently non-functional for simulation/ (see ComponentManager.cpp).

The hotloading you mean (doesn't refer to that mechanism) and is indeed currently impossible by the deepfreeze.

I would propose that the entire thing should be reloaded on init and we should still have to only read from memory.

Notice that hotloading templates should not only work for templates which started to exist but also changes in template data.
(Bit of a can of worms changing tempalte data in the simulation however)

I could reproduce the hotloading effect in the simulation, so if the file is reloaded, it should call init again. globalscripts should become able to do that too.
(i.e. diff correct in all cases afaiks)

Since I didn't post it yet, the DataTemplateManager system component should become globalscripts/vfs.js and look like this P97 which removes this inconsitency as well.
This will also make it easier to implement template hotloading.
Thanks for taking the time and having it pointed out!

Closed by commit rP20587: Check for Technology existence by looking up loaded technologies rather than… (authored by elexis). · Explain WhyDec 5 2017, 12:58 AM

This revision was automatically updated to reflect the committed changes.