boformer / BuildingThemes

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

Basic themes management. #10

Closed bloodypenguin closed 9 years ago

bloodypenguin commented 9 years ago

Changes:

bloodypenguin commented 9 years ago

This code wasn't tested yet. Also didn't change the project file.

boformer commented 9 years ago

Now we are kind of working on the same features. I like your approach with the separate "manager class'.

Look at my PR, I'm using a simple "List" array to store the themes enabled in a district.

bloodypenguin commented 9 years ago

Well, it was just a silly typo :) Everything's working now: http://screencast.com/t/FlxQ8V3aEm I think we should merge our branches because we're working on the same stuff at the same time.

bloodypenguin commented 9 years ago

Basic functionality is finished! At this point we only lack custom themes management feature. What do you think, do we need to finish Themes Management UI first or we can publish an alpha version now?

bloodypenguin commented 9 years ago

Not right now, of course. We need to check first that there are no errors on consequent saves and loads and also on exit to main menu and loading the game again to make sure that the mod initializes/disposes correctly.

boformer commented 9 years ago

Testing it now.

I would say let's clean up the source code a little bit (variable names, formatting, separate files for every class)

boformer commented 9 years ago

Update: Serialization isn't working properly.

Create 2 cities with different district/theme setups. Load the first city and check the themes, then load the second city without restarting the game.

The themes selected in the second city will be the same as in the first city.

I also discovered a bug in your euro buildings unlocker mod. It does not work when you load/create the second city. You have to restart the game to make it work again.

boformer commented 9 years ago

It seems like the configuration file is also not saved to disk.

boformer commented 9 years ago

Ok, tested it a bit more:

The serialization of the enabled themes per district works fine.

The deserialization also works fine for the first city loaded after the game is started.

When you load the second city, it loads the data saved for the first city.

When you load the third city, it loads the data saved for the second city. ...

boformer commented 9 years ago

This is a serious bug in the C:SL modding api.

It also happens when you do this:

ILoadingExtension:

void OnCreated(ILoading loading) 
{
    Debug.log(loading.currentTheme) // displays the theme of the city loaded before
}
bloodypenguin commented 9 years ago

Fix default config generation That wasn't a bug - I did it for reason. That Euro and International configurations are not supposed to be loaded from XML and be saved to config later (no duplicates). And that's why: Imagine you loaded the game with this mod the first time. Euro and Intl. themes get saved to XML. Then, in two months a new C:SL release comes out that adds more vanilla buildings to the game. Those buildings won't show up in your European and International themes because now the themes will only be loaded from XML. My point is: only user's custom themes should be saved/loaded to:

So, IMO Euro and International themes must always be loaded from memory and must not be saved to themes config. We should implement 'duplicate theme' functionality so that user will be able to 'edit' these themes without breaking them i.e. able to create a custom theme using built-in one as template. Custom themes should be saved though - only on changes to themes in Themes Manager UI.

boformer commented 9 years ago

You should not add the 2 themes to the list in the configuration.

With the ingame theme manager, the configuration has to be saved when the player changes a theme.

You could add a ThemeType enum and add it to the Theme Class, and only save entries with ThemeType.Custom

Another solution: Allow users to add their own buildings to euro/int theme (by adding them to the config), but don't add the Default buildings to the config file.

bloodypenguin commented 9 years ago

As you see, in my last commit I prevented built-in themes from being saved to XML and added "builtIn" flag. I also expect these themes to be immutable in themes manager UI. If user wants to add some building to a built-in theme he duplicates it (with a unique name) and adds building to the newly created theme.

boformer commented 9 years ago

Ok. Later (when the custom theme Manager is implemented) we Can add an option to disable the Default themes.

A detection if the euro buildings mod is enabled would also be nice

bloodypenguin commented 9 years ago

A detection if the euro buildings mod is enabled would also be nice That shouldn't be a problem. BTW, I have trouble reproducing that E.B.U. new game bug. The game freezes and then silently crashes on exit to main menu, even if no mods/assets installed. Do you have any idea, what might cause that?

boformer commented 9 years ago

I am testing the mods separately to see which one causes the issues.

bloodypenguin commented 9 years ago

Make sure you've pulled my latest commit in which I think I've fixed this bug:

Create 2 cities with different district/theme setups. Load the first city and check the themes, then load the second city without restarting the game. The themes selected in the second city will be the same as in the first city.

boformer commented 9 years ago

Got an exception:

The requested operation caused a stack overflow. [System.StackOverflowException]
Details:
No details
System.StackOverflowException: The requested operation caused a stack overflow.
  at BuildingThemes.DetoursHolder.ImmaterialResourceManagerAddResource (Resource resource, Int32 rate, Vector3 positionArg, Single radius) [0x00000] in <filename unknown>:0 
boformer commented 9 years ago

Looks like a random bug.

The theme loading now works correctly! Nice work!

bloodypenguin commented 9 years ago

Got an exception:

It looks like the detoured method detoured itself to itself :) May be some threading issue. Will check it.

boformer commented 9 years ago

Maybe it is because you don't revert the detour?

boformer commented 9 years ago

Testing a bit more. If this is stable, I will merge it.

boformer commented 9 years ago

I'm pushing my "revert detour" commit (tested). If you were working on the same feature, revert the commit.

boformer commented 9 years ago

Do you know why no configuration file is created?

bloodypenguin commented 9 years ago

It's in Steam\steamapps\common\Cities_Skylines. Where are you looking for it?

boformer commented 9 years ago

Oh right, there it is.

boformer commented 9 years ago

Merging this branch.