Convert braziers from traps to furniture

[Night-Pryanik]

• Replaced braziers as traps with braziers as furniture in churches (New England and Gothic styles), campsites, strange cabin, standing stones, cathedral and mapgen-test;
• Added recipe for placing brazier through the construction menu;
• Slightly changed description of brazier as tool to help understand how it should be placed;
• Remove check for brazier as a trap and replace it with check for furniture with FIRE_CONTAINER flag in field.cpp;
• Remove mentions of brazier as a trap for traps.json, trap.cpp and trap.h.

[Coolthulhu]

That simplifies things.

Though one thing I'd rather keep: the ability to deploy it with an action. place_trap actor is very simple and you could easily copy it, cut out most trap-related code and create place_furniture out of the rest.
That's not strictly mandatory for me, but dropping that functionality is a regression, so others may disagree.

Save migration of old trap braziers to the new furniture ones, however, is mandatory.

[Coolthulhu]

Why does it deconstruct into parts instead of back into brazier?

[Night-Pryanik]

I wasn't sure which path is better - deconstruct it to brazier item or directly to components. Now that you mention it, I'll change it.

[Night-Pryanik]

Save migration of old trap braziers to the new furniture ones, however, is mandatory.

You mean through creating legacy file, putting deleted info from traps.json to it, and then raising game core version?

[Coolthulhu]

You mean through creating legacy file, putting deleted info from traps.json to it, and then raising game core version?

If you do it that way, you will also need to restore the handling in fields.cpp, preferably with a comment indicating that it's only for pre-0.D saves.

[Night-Pryanik]

If you do it that way, you will also need to restore the handling in fields.cpp, preferably with a comment indicating that it's only for pre-0.D saves.

Is there other way to do migration?

[Coolthulhu]

You could also detect it in mapbuffer.cpp, in the mapbuffer::deserialize, and convert it there.

[Reclusive-reptile]

So now we will need a hammer and screwdriver to take down a brazier?

[Night-Pryanik]

So now we will need a hammer and screwdriver to take down a brazier?

Yes, like all furniture.

[DangerNoodle]

So now we will need a hammer and screwdriver to take down a brazier?

That is a problem, yes. I attempted to devise code to allow taking down select furniture without tools, but was unable to devise an effective way to implement it.

[Night-Pryanik]

You could also detect it in mapbuffer.cpp, in the mapbuffer::deserialize, and convert it there.

This means that if I'm standing near brazier trap, save game, make changes in mapbuffer and compile, then trap will convert to furniture after I load a save?

[Coolthulhu]

This means that if I'm standing near brazier trap, save game, make changes in mapbuffer and compile, then trap will convert to furniture after I load a save?

Yes.

[BorkBorkGoesTheCode]

Is there any way to make furniture carryable?

[kevingranade]

You have to add a method to transform the furniture into an item.

[DangerNoodle]

For this I would suggest that @Night-Pryanik attempt to implement some method of easier deconstruction (as I tried to in #18346).

[Coolthulhu]

Implementing it on an iexamine method wouldn't be hard.
I'd do it like this: iexamine method deconstruct_furn or take_down_furn, which would query the player, remove the furniture on a given tile, and drop its deconstruct items under the player (under player and not where furniture was, because harvest already drops under player).

[Night-Pryanik]

@Coolthulhu I think mapbuffer way you mentioned is much more cleaner than legacy way, but I'm afraid I'm doing something wrong while trying to implement it. Could you or anyone else please help me here?
I tried to change a piece of mapbuffer to this:

} else if( submap_member_name == "traps" ) {
    jsin.start_array();
    while( !jsin.end_array() ) {
        jsin.start_array();
        for( int j = 0; j < SEEY; j++ ) {
            for( int i = 0; i < SEEX; i++ ) {
            const trap_str_id trid( jsin.get_string() );
                if( trid == "tr_brazier" ) {
                    sm->frn[i][j] = furn_id( "f_brazier" );
                } else {
                    sm->trp[i][j] = trid.id();
                }
            }
        }
        jsin.end_array();
    }
}

But this doesn't seems to work.

[Zireael07]

"jsin" looks suspiciously like a typo for "json"...

[Coolthulhu]

What do you mean "doesn't seem to work"? Doesn't compile or doesn't do what it should?

If it fails to compile at

if( trid == "tr_brazier" ) {

try wrapping the string in an ID so that you compare and ID with an ID.

[Night-Pryanik]

It successfully compiles, but doesn't convert trap to furniture.

[Night-Pryanik]

I've taken a save from my old game, where character is standing next to brazier trap. I've copied this save to my branch where brazier is furniture and no trace of tr_brazier at all. I loaded this save and it does not give me any errors at the start.

[Coolthulhu]

Oh, I see it now. Why do you iterate over the whole submap? You only need to replace this line:

sm->trp[i][j] = trap_str_id( jsin.get_string() );

with something like

const trap_string_id trp_id( jsin.get_string() );
if( trp_id != "t_brazier ) {
sm->trp[i][j] = trp_id;
} else {
sm->frn[i][j] = f_brazier;
}

[Coolthulhu]

Also comment the conversion, with something like @todo: remove after 0.D.

[Night-Pryanik]

Done in 158ff80.

[Night-Pryanik]

It worked! Thanks @Coolthulhu.

As for deconstructing selected furniture don't require screwdriver and hammer - I think it should be in some other PR. The same for carryable furniture.

[Coolthulhu]

As for deconstructing selected furniture don't require screwdriver and hammer - I think it should be in some other PR. The same for carryable furniture.

Depending on what others say, it may be required for those PRs to happen first.
I wouldn't put this as a strict requirement because I don't recall moving braziers except when doing exploits, but requiring construction menu for that is a regression.

[DangerNoodle]

As for deconstructing selected furniture don't require screwdriver and hammer - I think it should be in some other PR. The same for carryable furniture.

Depending on what others say, it may be required for those PRs to happen first.

I do advocate in favor of some way to take down braziers without using deconstruction. Though I will admit that is not just because it avoids a regression in player convenience, but due to how useful the feature would be for other purposes.

[Coolthulhu]

It's actually just "code aware copy+paste" tier in difficulty of writing (both deployment and taking down), the only problem here is the feature freeze.

[kevingranade]

This doesn't fix #21283. That issue is about stoves, not braziers. The thing about converting braziers is a side issue, and a very minor one at that.

Disassembly requiring arbitrary new tools for braziers is an unacceptable regression for the release, feel free to implement it in a new PR, but this one should not be merged until that is available. Frankly I don't see merging this before 0.D as it doesn't fix a major issue.

[Night-Pryanik]

Disassembly requiring arbitrary new tools for braziers

That's not about disassemble, but about deconstruction.

As for #21283, you are right, I was wrong. But if this PR is not a fix for this issue, but a standalone feature, will this have a chance to get merged?

[kevingranade]

It's a very minor feature addition (make brazier draggable), so its not going to be accepted if it has any kind of regression (i.e. requiring tools to didassemble deployed brazier).
If it's regression free and handles the migration properly, it can probably go in before 0.D in theory, but it's going to be pretty low on the list since it's both a bit complex and a minor feature.

[Night-Pryanik]

Is this a right way to avoid regression?

[DangerNoodle]

Is it really that simple, just adding a new flag for pre_flags to check? I will feel rather stupid if that is the case.

[DangerNoodle]

I just tested this change. It does indeed work as expected. I will make use of this as soon as I am able to.