boformer / BuildingThemes

Mod for Cities:Skylines
http://steamcommunity.com/sharedfiles/filedetails/?id=466158459
MIT License
13 stars 11 forks source link

Detour methods that call GetRandomBuildingInfo #2

Closed bloodypenguin closed 9 years ago

bloodypenguin commented 9 years ago

Have finished methods implementations, but ran into problem. It looks like simple patching won't work for virtual methods and jit-table needs to patched too in order for these 3 methods to be called. I will rewrite redirection using this method from original detours library my tomorrow. Hopefully it's what we need here.

bloodypenguin commented 9 years ago

But GetRandomBuildingInfo is redirected. I got lots of such repeating lines in output_log when running the game:

Building Themes: Detoured GetRandomBuildingInfo. position: (0.0, 0.0, 0.0)
Building Themes: Detoured GetRandomBuildingInfo. districtIdx: 0
bloodypenguin commented 9 years ago

Well, detours library doesn't work for virtual methods. Fortunately, ZoneBlock's SimulationStep method isn't virtual. Also I've found another way to pass position on building upgrade - using LevelUpExtensionBasemethods.

So, at this point I pass position for these two use cases: ZoneBlock.SimulationStep - Places new growables on empty land PrivateBuildingAI.GetUpgradeInfo - Finds a new growable of the same size (current level +1)

It's quite enough for building themed districts. I still need to find a way to pass position for this use case:

PrivateBuildingAI.SimulationStep - "Downgrades" abandoned buildings to level1 buildings

But this case it pretty rare if a city functions normally. If I won't find a way, we can just tell users to use Auto Bulldoze mod to prevent this case at all :)

What really surprised me today is that I've found 4th use case of GetRandomBuildingInfo. Render thread uses PrivateBuildingAI.GetUpgradeInfo too when rendering buildings under upgrade. At this point I have no idea why it does so (see stack trace below) and will investigate this later. Do you have any ideas why?

I intend to store simulation thread id and don't call our custom code in GetRandomBuildingInfo if we're in different thread to prevent race condition.

Here is the stack trace that I've mentioned:

Building Themes: Detoured GetRandomBuildingInfo was called. Stack trace:    at System.Environment.get_StackTrace()
   at BuildingThemes.DetoursHolder.GetRandomBuildingInfo(Randomizer ByRef r, Service service, SubService subService, Level level, Int32 width, Int32 length, ZoningMode zoningMode)
   at PrivateBuildingAI.GetUpgradeInfo(UInt16 buildingID, Building ByRef data)
   at CommonBuildingAI.RenderInstance(.CameraInfo cameraInfo, UInt16 buildingID, Building ByRef data, Int32 layerMask, Instance ByRef instance)
   at Building.RenderInstance(.CameraInfo cameraInfo, UInt16 buildingID, Int32 layerMask, .BuildingInfo info, Instance ByRef data)
   at Building.RenderInstance(.CameraInfo cameraInfo, UInt16 buildingID, Int32 layerMask)
   at BuildingManager.EndRenderingImpl(.CameraInfo cameraInfo)
   at SimulationManagerBase`2.EndRendering(.CameraInfo cameraInfo)
   at BuildingManager.IRenderableManager.EndRendering(.CameraInfo cameraInfo)
   at RenderManager.LateUpdate()
boformer commented 9 years ago

Why don't you just completely reinplement the methods calling GetRandomBuildingInfo?

Just call a different method with an additional district parameter.

bloodypenguin commented 9 years ago

Because PrivateBuildingAI.SimulationStep and PrivateBuildingAI.GetUpgradeInfo are virtual methods and the detours library doesn't allow to override virtual methods.

bloodypenguin commented 9 years ago

Looks like I've found a good solution how to get buildingId for PrivateBuildingAI.GetUpgradeInfo. I've noticed that when GetRandomBuildingInfo is called from PrivateBuildingAI.SimulationStep and ZoneBlock.SimulationStep, Singleton<SimulationManager>.instance.m_randomizer is used as randomizer argument. But when GetRandomBuildingInfo is called from PrivateBuildingAI.GetUpgradeInfo, a newly constructed randomizer is passed and buildingId is used as its seed. So, in out detoured version of GetRandomBuildingInfo we need to compare randomizer seed with SimulationManager's randomizer seed. If they are not equal it means that randomizer's seed is buildingId. Having builldingId we can retrieve its position:)

bloodypenguin commented 9 years ago

I'll see if that helps

boformer commented 9 years ago

I noticed that a while ago, see point 3 in my comment: https://github.com/boformer/BuildingThemes/commit/3aa1fd0bbb8c0bccac469e6c760aa6639f9d736e#commitcomment-11618110

bloodypenguin commented 9 years ago

It looks like it's impossible to override Randomizer's constructor as you suggested. I've tried that option but have failed. What I need now is how to restore original _seed argument of Randomizer's constructor knowing value of its seed field.

    public Randomizer(uint _seed)
    {
      this.seed = (ulong) (6364136223846793005L * (long) _seed + 1442695040888963407L);
    }
boformer commented 9 years ago

Alternatively we could override the Residential/Office/Industrial/CommercialBuildingAI until we found a better solution.

It is quite simple because the AI instances are independent parts of code and not referenced anywhere else.

bloodypenguin commented 9 years ago

I din't like that option, but now I think it's worth trying :) Will do that tomorrow.

bloodypenguin commented 9 years ago

My algorithm seems to be working now for all of `GetRandomBuildingInfo use cases :) Now I want to test it and need some filtering implementation.

bloodypenguin commented 9 years ago

This is how log entries look like for PrivateBuildingAI.GetUpgradeInfo: Building Themes: Getting position from seed 1174684194502217932. building: L2 2x2 Detached01, buildingId: 913, position: (754.6, 97.3, -33.8), threadId: 1

bloodypenguin commented 9 years ago

I go to sleep now. Will continue to check if my algorithm works correctly tomorrow.

bloodypenguin commented 9 years ago

Tested is today. It's mostly working. I've implemented a stub filter so that buildings within districts are european and buildings outside of districts are international. Some buildings grow out of place and it looks like the more growables in town the bigger part of them grow out of place. See here: http://screencast.com/t/YjrGklLqT81j

bloodypenguin commented 9 years ago

These lines appear in my log (they appear when position is null): Building Themes: Detoured GetRandomBuildingInfo. No position was specified!;current thread: 2 If they appear that means that the list won't be filtered. I think that's why some buildings grow out of place. Let's see what use case is causing that...

bloodypenguin commented 9 years ago

It wasn't a good idea to wrap position and to nullify the wrapper after its use indeed. ZoneBlock.SimulationStep calls GetRandomBuildingInfo multiple times (one time per building). So when I nullified I disabled filtering for the rest of the buildings in the block after processing the first one. After removing wrapper everything works fine now :) http://screencast.com/t/WLQAVyren1Az Still need to find a way to pass position for case when an abandoned building's plot gets reclaimed though. But that's a minor issue at this point.

boformer commented 9 years ago

I also tested it yesterday. The abandoning was the only thing that did not work correctly.

Extending the AIs to override the virtual methods would solve the problem.

Do you know another plugin that does that?

Post the screenshot on reddit ;)

bloodypenguin commented 9 years ago

I think we don't need to override virtual methods. I'm digging SimulationStep now to find a function which arguments may be building, its position or its. Anyway, I read about VTables today. It's possible to patch them in C++. Hope, it's also possible to patch them in C#.

Posted on Reddit here

bloodypenguin commented 9 years ago

I've loaded my savegame of a decently-sized city and... oh man! what a performance hit this mod is. It's too much even for my Intel4790k / 16GB RAM. I think optimization is my main priority now.

boformer commented 9 years ago

The filter method might be a problem. It would be better to precompile the list. I am working on that.

The LevelUpExtension is also called frequently. That's why I thought it would be better to override the AI methods, which should not result in a performance hit.

Also try to avoid reflection where possible.

bloodypenguin commented 9 years ago

Caching of MethodInfo's and pointers to functions saved my day. Looks like reflection is much bigger performance killer than filtering. With logs disabled I observe no lags now at all, even for extremely large cities such as this one (at least at my system specs). The game runs smoothly even when there are lots of levelups.

bloodypenguin commented 9 years ago

Have found a simple and effective solution to handle abandoned buildings case. Still no performance issues observed. After some cleanup I'm ready to merge. Test this branch please when you have some spare time. Which feature would you like me to do next? Maybe you need some help with UI?

boformer commented 9 years ago

A way to save the data in the game save files would be important (which themes are enabled city-wide/in a district).

bloodypenguin commented 9 years ago

Good. I would prefer to save all data in XML like Traffic Manager does so that nobody's saves would be corrupted.

boformer commented 9 years ago

Testing right now. Can't get any european buildings to spawn.

boformer commented 9 years ago

Seems like the problem is that there are no level1 prefabs with "lock" or "EU" in the name.

boformer commented 9 years ago

Excellent! Everything works!