Full Window Audit

[souricelle]

This PR has been closed.

[kevingranade]

Looks like some faction camp manifests have gone out of sync now, should be pretty easy to clear up, but I don't have time to do so at the moment.

[souricelle]

I'll do some testing and see if I can get it fixed, or at least document what exactly needs to get fixed. Thanks for the heads up.

[Night-Pryanik]

I don't like your removal of single-pane, double-pane etc windows. Even if they don't used anywhere in the mapgen, players could build them by themselves.

Since you're changing ids of the terrain, you need to apply migration for old ids into a new one. Otherwise there would be LOTS of errors on game load.

[souricelle]

I don't like your removal of single-pane, double-pane etc windows. Even if they don't used anywhere in the mapgen, players could build them by themselves.

Since you're changing ids of the terrain, you need to apply migration for old ids into a new one. Otherwise there would be LOTS of errors on game load.

I really don't think anyone was using them at all. They were completely pointless and didn't even work, and we'd have had bug reports if people had made these faulty windows that couldn't be used properly. They're also redundant - is a normal house window single pane? double pane? Additionally per Candlebury, a quadruple pane window isn't really something that would exist outside of serious custom jobs, and since there's no gameplay effect I can't think of a single argument for keeping 45 window objects that nobody was using. If you haven't, please take a look at the linked PRs and understand how badly mangled and nonsensical the jsons were - they should never have been merged to begin with and the only reason I didn't just propose a revert was because I couldn't find them.

I can use migration IDs, we could even migrate all the single/double/triple pane windows to just being normal house windows since they're functionally identical (except normal house windows actually work). I'm going to go out on a limb and suggest that people probably also weren't using the grate or plastic windows as those didn't work properly either, but if I'm wrong I'm sure they'll hardly mind one or two of their windows changing names on an update (and acquiring proper function and tiles).

[souricelle]

• Added migration definitions for 45 removed windows. All single/double/triple/quadruple pane windows just become the normal "domestic" window they were copied from and identical to, and the "reinforced plastic" window becomes the plastic window it was identical to. The grated windows and their curtained cousins are now barred reinforced windows of the appropriate type with no curtains, because they're opaque. Migrations were also added to cover ID changes

[Night-Pryanik]

Unfortunately, this approach won't work. migration only works for items. You need to do the whole work in the source code.

[souricelle]

Unfortunately, this approach won't work. migration only works for items. You need to do the whole work in the source code.

I wouldn't even know where to begin with that. I'd appreciate some more detailed direction but my C++ skills are very limited.

However in testing, loading old saves that have built the old versions of the windows into a game with these changes made doesn't give any errors. The old windows simply exist as they were, while the player has the option to build new ones. You can even open and close the old windows and when you break them they give their old smash yields. I'm not sure I see what the issue is in that case.

Here's my test character. To her right is a "plastic window" that she just built after I rolled in the update. Down south are a "Plastic window" and the other outdated kinds she built before I rolled in the update, they loaded in perfectly well with no errors.

I don't see the need for any migration work since the game seems to handle it perfectly well on its own.

[Night-Pryanik]

Just look at the output of failed "general build matrix". There are already tons of errors. It's failed exactly because of that - the game can't find old IDs of the terrain you've changed.

[souricelle]

Can that be avoided by leaving the old IDs up, apart from the ones we're removing? It's messier but I don't know how to fix that in C++

Failing that, can we simply revert thegoatgod's two PRs?

[Night-Pryanik]

I hope this will help you

Cataclysm-DDA/src/savegame_json.cpp

Lines 4117 to 4137 in fa0b7d6

	if( tid == ter_t_rubble ) {
	ter[i][j] = ter_id( "t_dirt" );
	frn[i][j] = furn_id( "f_rubble" );
	itm[i][j].insert( rock );
	itm[i][j].insert( rock );
	} else if( tid == ter_t_wreckage ) {
	ter[i][j] = ter_id( "t_dirt" );
	frn[i][j] = furn_id( "f_wreckage" );
	itm[i][j].insert( chunk );
	itm[i][j].insert( chunk );
	} else if( tid == ter_t_ash ) {
	ter[i][j] = ter_id( "t_dirt" );
	frn[i][j] = furn_id( "f_ash" );
	} else if( tid == ter_t_pwr_sb_support_l ) {
	ter[i][j] = ter_id( "t_support_l" );
	} else if( tid == ter_t_pwr_sb_switchgear_l ) {
	ter[i][j] = ter_id( "t_switchgear_l" );
	} else if( tid == ter_t_pwr_sb_switchgear_s ) {
	ter[i][j] = ter_id( "t_switchgear_s" );
	} else {
	ter[i][j] = tid.id();

[souricelle]

Thanks! That is actually just what I needed. It looks like it's going to wind up being a lot of lines so I hope that's not a major issue, I know we try to keep the c++ trim.

[Night-Pryanik]

In theory we could only keep all this migration code snippet only up to the next stable (0.F. in this case). After that, we could get rid of it.

[souricelle]

This isn't happening until after string freeze anyway, I had to fix a bunch of descriptions.

[souricelle]

Whew, that was a lot of work. If anyone has any further input let me know, but otherwise this one's done.

[Night-Pryanik]

@souricelle will this fix #39720?

[souricelle]

@souricelle will this fix #39720?

Yep!

[Night-Pryanik]

Why closing?

[souricelle]

Why closing?

I elected to delete my fork and close my unmerged PRs following a personal dispute with Kevin. If my code is used, it is being done against my wishes.

He seems to think he has some moral imperative to use it, but frankly I find that pretty gross.

[Night-Pryanik]

That's a shame.
Well, good luck to you!

[souricelle]

Sorry to disappoint, and thanks, you too.

[ZhilkinSerg]

Why closing?

I elected to delete my fork and close my unmerged PRs following a personal dispute with Kevin. If my code is used, it is being done against my wishes.

He seems to think he has some moral imperative to use it, but frankly I find that pretty gross.

Luckily that is not how licenses work.