jptrrs / HumanResources

A technology overhaul for RimWorld
MIT License
17 stars 12 forks source link

DNM: Implement active caching for weapons and growables #153

Open Itaros opened 3 years ago

Itaros commented 3 years ago

WARNING: DO NOT MERGE - PROOF OF CONCEPT STAGE

This shows possibility of implementing active caching which may be used in JobGivers to optimize lookups. It is not perfect as it is quite destructive to the code in its current form: from start SRP is not in greatest shape, but after my changes it looks like pigsty, which may result in those changes actually breaking stuff, but this topic is addressed in the end of the description.

Related to: https://github.com/jptrrs/HumanResources/issues/67

Comparisons:

Original steam master: og

With changes, but cache is disabled: changed_no_cache Oh no...

With changes, but cache is active: changed_active_cache

Disclaimer: Those measurements were made in heavily modded game manually with the same capture scenario and under the cases of similar call per sampling range from the Dubs profiler. Therefore those are not perfect, but I hope indicative enough.

This little endeavor showed me the following:

About code quality with optimizations:

I propose a refactor, especially for the base mod class, unlocks database and knowledge comp. I really would like to turn those classes into rimworld interface; and core logic about unlocks and selections extracted into separate agnostic classes. Should I? If not, I hope at least those changes would be useful for independent consideration.

jptrrs commented 3 years ago

It seems you were able to successfully navigate my code, figure out what is what and design a reasonable interference. I only recently learned how to use an event properly, so I simply could not have devised a caching implementation like that when I was writing this. So it looks neat! If you believe a refactor can make it better, I'm not the one who's going to stop you. Go ahead!! If you look into the recent repo history, you'll see I have been reconsidering the UnlockManager myself, having moved a lot of logic that used to live there to the static and more specialized TechTracker. I'm glad you didn't use the current Steam version, BTW, as I already moved past that.