Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Portable smoking rack #29915

Merged
merged 19 commits into from
May 8, 2019
Merged

Conversation

Scischm
Copy link
Contributor

@Scischm Scischm commented Apr 25, 2019

Summary

SUMMARY: Features "Adds a deployable smoking rack."

Purpose of change

Creates an option for nomadic players to quickly setup/disassemble smoking racks similar to the deployable tourist table, tarps and mats, and metal butchering rack. Smoking racks are quickly becoming the best way to handle large amounts of quickly rotting food for long term storage and the charcoal smoker doesn't have a similar purpose to the smoking rack. The process of building and deconstructing regular smoking racks added to tedium, and the long construction times often resulted in a large loss of meat freshness or spoilage unless you immediately find a way to freeze the whole corpse.

Describe the solution

The solution was to implement a new deployable item based off the existing template for the metal butchering rack.
This required some tweaking to the current smoker code. Being a baby coder I tried my best to ensure that the existing code for the smoker handled the heavy lifting. I mostly implemented several if statements to recognize and appropriately change between the f_metal_smoking_rack and f_metal_smoking_rack_active furnitures. The existing deploy_furniture function is used for both placing and pickup, and when deconstructed yields the metal smoking rack item itself.

Describe alternatives you've considered

Other alternatives include the addition of vehicle parts that serve similar purposes, or adding the smoker menu with a much lower capacity volume to the charcoal smoker item. If possible I would like to look into implementing large refrigerated and freezer storage via a recipe that users several minifreezers/minifridges and a cargo container. For the moment however, this got my feet wet with using github and exploring the existing code of C:DDA.

metal smoking rack
deploy
deconstruct
smash
smoking

@Scischm
Copy link
Contributor Author

Scischm commented Apr 25, 2019

I also greatly appreciate feedback on how to better format my code.

Also, it seems I failed some of the autochecks. I'm not sure where to begin fixing the problems, any help would be greatly appreciated!

@thquinn
Copy link
Contributor

thquinn commented Apr 25, 2019

You have to lint your JSON files: http://dev.narc.ro/cataclysm/format.html

src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
@Night-Pryanik
Copy link
Contributor

Night-Pryanik commented Apr 25, 2019

Did you test your changes? All these or instead of || should cause all sorts of errors when trying to compile.

@Scischm
Copy link
Contributor Author

Scischm commented Apr 25, 2019

I went ahead and made the suggested changes. Thank you very much.

Copy link
Contributor

@Night-Pryanik Night-Pryanik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Full astyle of all your changes required.

src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
src/iexamine.cpp Outdated Show resolved Hide resolved
@Scischm
Copy link
Contributor Author

Scischm commented Apr 25, 2019

Things were looking good until an error was thrown. It doesn't seem to be related to anything that I changed though. Unsure how to progress.
image

@AMurkin
Copy link
Contributor

AMurkin commented Apr 25, 2019

We use 4 spaces to indent code. Don't use tabs.
.astylerc in the project's root folder contains options for the astyle.

@kevingranade
Copy link
Member

Other alternatives include the addition of vehicle parts that serve similar purposes

I'd prefer to avoid this unless we can do something to represent the fact that you potentially end up with a fire and tons of smoke in a moving vehicle, which sounds... unsafe.

or adding the smoker menu with a much lower capacity volume to the charcoal smoker item.

I think this needs to happen, leaving the portable version with the same capacity makes it a strictly superior option to the existing construction.

@Scischm
Copy link
Contributor Author

Scischm commented Apr 26, 2019

I think this needs to happen, leaving the portable version with the same capacity makes it a strictly superior option to the existing construction.

Did you mean only having a smaller capacity handheld charcoal smoker item?

Because what I currently have implemented is a full sized metal rack that is deployable in the same vein as the metal butchering rack or tourist table. What I had meant by adding the smoking rack menu to the charcoal smoker was the handheld item in addition to the current changes with the metal deployable rack, possibly in a future PR.

We can adjust the numbers as necessary including volume reduction on the deployable version but I felt it was meant to be a superior option to the original construction due to the requirement of metalworking tools.

I just took a look at the current requirements for the craft of the deployable racks, both butchering and smoking and they require an anvil and hacksaw, but no need for a welding torch, goggles etc. Should the barrier to entry for their creation include welding tools to be a bit higher in order to offset their value?

My thought was that the slightly less restrictive requirements might help out less developed characters get a bit of breathing room when it came to the topic of food preservation. They also still have to supply the charcoal to keep the racks going which is an undertaking in itself.

@l29ah
Copy link
Contributor

l29ah commented Apr 26, 2019

I don't see why would it have a reduced effective volume compared to a thing that is made of sixteen sticks and eight rocks.

@ifreund ifreund added <Enhancement / Feature> New features, or enhancements on existing [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. labels Apr 26, 2019
@kevingranade
Copy link
Member

Did you mean only having a smaller capacity handheld charcoal smoker item?

I don't know what you mean by handheld, if you mean luggable/portable, yes the portable would be more restricted than the fixed one. If you mean an item you carry around while operating it, that shouldn't even exist.

@Scischm
Copy link
Contributor Author

Scischm commented Apr 27, 2019

I don't know what you mean by handheld, if you mean luggable/portable, yes the portable would be more restricted than the fixed one. If you mean an item you carry around while operating it, that shouldn't even exist.

I reduced the capacity of the deployable (portable) smoking rack to 15 liters (30 chunks of meat) down from 20 liters (40 chunks of meat). I slightly reduced the volume of the rack to reflect this, and realistically the volume wouldn't be that high while the rack is folded since it doesn't have the charcoal or meat in/on it.

@ZhilkinSerg ZhilkinSerg self-assigned this May 1, 2019
@Scischm
Copy link
Contributor Author

Scischm commented May 3, 2019

I think I found the problem

bool item::process_fake_smoke( player * /*carrier*/, const tripoint &pos )
{
    if( g->m.furn( pos ) != furn_str_id( "f_smoking_rack_active" ) && != furn_str_id( "f_metal_smoking_rack_active" ) ) {
        item_counter = 0;
        return true; //destroy fake smoke
    }

    if( age() >= 6_hours || item_counter == 0 ) {
        iexamine::on_smoke_out( pos, birthday() ); //activate effects when timers goes to zero
        return true; //destroy fake smoke when it 'burns out'
    }

    return false;
}

process_fake_smoke() in a completely different source file and was destroying the fake smoke before I added the metal smoking rack to its whitelist.

@Scischm
Copy link
Contributor Author

Scischm commented May 3, 2019

There were also a few lines that were missing "\n " causing some wonky formatting. Since the mills were copied off of the smoking rack it might be prudent to check their output and change it, but I didn't want to go outside the scope of this. (Unless the newlines were missing for a reason I'm not aware of because I'm a newer coder.)

@ZhilkinSerg ZhilkinSerg merged commit f0362ae into CleverRaven:master May 8, 2019
@ZhilkinSerg ZhilkinSerg removed their assignment May 8, 2019
@jbytheway
Copy link
Contributor

Did you test your changes? All these or instead of || should cause all sorts of errors when trying to compile.

In C++ or and || are equivalent; both work (but the former is rarely used).

@codemime
Copy link
Contributor

codemime commented May 9, 2019

In C++ or and || are equivalent; both work (but the former is rarely used).

To the best of my knowledge, or isn't even mentioned in the standard. It won't compile in MSVC, for example.

@ZhilkinSerg
Copy link
Contributor

It is in standard at least from C++11:

image

We can disable language extensions and they should work:

image

@codemime
Copy link
Contributor

It turns out you're right. I wasn't aware of their legitimacy. I thought they're some kind of freakish language extensions.

@ZhilkinSerg
Copy link
Contributor

It turns out you're right. I wasn't aware of their legitimacy. I thought they're some kind of freakish language extensions.

Yes, it is kinda funny, that MSVC which supports things foreach in their C++ compiler extensions uses same extensions to disable or.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C++] Changes (can be) made in C++. Previously named `Code` <Enhancement / Feature> New features, or enhancements on existing Fields / Furniture / Terrain / Traps Objects that are part of the map or its features. [JSON] Changes (can be) made in JSON
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants