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

Fix Hotplate Segfault #74245

Closed
wants to merge 3 commits into from
Closed

Conversation

ShnitzelX2
Copy link
Contributor

Summary

Bugfixes "fixes hotplate segfaults, adds pre-#72514 hotplate to oven"

Purpose of change

Patches #73657, #74107, #74023

The segfaults should be fixed, there were some missing return statements after setting the heating activity to null.

The broader problem: #72514 did not design around built-in hotplates in furniture/VPs. Specifically, the pseudo_tool instantiated in use_vehicle_tool() (for e.g. an oven) does not persist until heat_activity_actor::do_turn().

I don't know how to best fix that, and I'd want author @yuganxia to comment. For now, the oven gets the older hotplate functionality. The RV kitchen unit has a removable hotplate, so I'm not touching it.

To be clear, batch cooking still will NOT work when using a pseudo_tool (e.g. oven or kitchen RV unit); you'll just get "can't find heating source" instead of a segfault.

Describe the solution

See above

Describe alternatives you've considered

Completely rewriting batch cooking, but this will suffice for now

Testing

-Made sure using an oven and batch cooking didn't cause a segfault.
-Tested batch cooking on a low battery and made sure the correct error message displayed.
-Tested using oven hotplate and then exiting menu (this was also causing segfaults)

Additional context

@github-actions github-actions bot added [JSON] Changes (can be) made in JSON [C++] Changes (can be) made in C++. Previously named `Code` Appliance/Power Grid Anything to do with appliances and power grid <Bugfix> This is a fix for a bug (or closes open issue) json-styled JSON lint passed, label assigned by github actions astyled astyled PR, label is assigned by github actions labels Jun 1, 2024
@yuganxia
Copy link
Contributor

yuganxia commented Jun 2, 2024

I see, i'll check it later, Thx.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Jun 2, 2024
@yuganxia
Copy link
Contributor

yuganxia commented Jun 2, 2024

It's kind of tricky...There's no good way to handle pseudo_tool now.
I need rewrite some of the heat_items func, I'll take it from here, plz wait.

@chefoxara

This comment was marked as off-topic.

oven should still allow crafting items with a hotplate
@ShnitzelX2
Copy link
Contributor Author

Awesome. I assume you'll include the segfault fix in a different PR, so I'm going to draft this PR for now. Thanks!

I'd recommend anyone who wants the issue fixed to merge this branch into their own for the time being.

@ShnitzelX2 ShnitzelX2 marked this pull request as draft June 2, 2024 19:32
@RenechCDDA
Copy link
Member

RenechCDDA commented Jun 2, 2024

I don't love the patch, but it looks okay. The missing returns are definitely a good thing to solve.

Specifically, the pseudo_tool instantiated in use_vehicle_tool() (for e.g. an oven) does not persist until heat_activity_actor::do_turn().

I think you can use vpart_position::get_tools() and construct a new item_location in place? Something like this:

(note that cached_itype_id will need to be passed into heat_activity_actor when it is started and existing activities loaded safely somehow. This patch is just for handling the vehicle tools in general)

index 10e41e47fd..5392aa4f7f 100644
--- a/src/activity_actor.cpp
+++ b/src/activity_actor.cpp
@@ -104,6 +104,7 @@
 #include "value_ptr.h"
 #include "veh_type.h"
 #include "vehicle.h"
+#include "vehicle_selector.h"
 #include "vpart_position.h"
 #include "vpart_range.h"
 
@@ -7686,6 +7687,23 @@ void heat_activity_actor::start( player_activity &act, Character & )
 
 void heat_activity_actor::do_turn( player_activity &act, Character &p )
 {
+    if( !h.loc ) {
+        if( h.loc.where() == item_location::type::vehicle ) {
+            // First we try to reconstruct
+            tripoint part_location = h.loc.position();
+            const optional_vpart_position vp = get_map().veh_at( part_location );
+            if( vp ) {
+                auto vp_ref = vp.part_with_tool( cached_itype_id );
+                auto tools_map = vp_ref->get_tools();
+                for( std::pair<item, int> some_item : tools_map ) {
+                    if( some_item.first.typeId() == cached_itype_id ) {
+                        item_location new_loc( vehicle_cursor( vp->vehicle(), vp->part_index() ), &some_item.first );
+                        h.loc = new_loc;
+                    }
+                }
+            }
+        }
+    }
     if( !h.loc ) {
         p.add_msg_if_player( _( "You can't find the heater any more." ) );
         act.set_to_null();

Note I think we may be making a copy here? I don't have a lot of time/willpower to look into this, but if you're interested.
for( std::pair<item, int> some_item : tools_map ) {

@yuganxia yuganxia mentioned this pull request Jun 4, 2024
@yuganxia
Copy link
Contributor

yuganxia commented Jun 6, 2024

I don't love the patch, but it looks okay. The missing returns are definitely a good thing to solve.

Specifically, the pseudo_tool instantiated in use_vehicle_tool() (for e.g. an oven) does not persist until heat_activity_actor::do_turn().

I think you can use vpart_position::get_tools() and construct a new item_location in place? Something like this:

(note that cached_itype_id will need to be passed into heat_activity_actor when it is started and existing activities loaded safely somehow. This patch is just for handling the vehicle tools in general)

index 10e41e47fd..5392aa4f7f 100644
--- a/src/activity_actor.cpp
+++ b/src/activity_actor.cpp
@@ -104,6 +104,7 @@
 #include "value_ptr.h"
 #include "veh_type.h"
 #include "vehicle.h"
+#include "vehicle_selector.h"
 #include "vpart_position.h"
 #include "vpart_range.h"
 
@@ -7686,6 +7687,23 @@ void heat_activity_actor::start( player_activity &act, Character & )
 
 void heat_activity_actor::do_turn( player_activity &act, Character &p )
 {
+    if( !h.loc ) {
+        if( h.loc.where() == item_location::type::vehicle ) {
+            // First we try to reconstruct
+            tripoint part_location = h.loc.position();
+            const optional_vpart_position vp = get_map().veh_at( part_location );
+            if( vp ) {
+                auto vp_ref = vp.part_with_tool( cached_itype_id );
+                auto tools_map = vp_ref->get_tools();
+                for( std::pair<item, int> some_item : tools_map ) {
+                    if( some_item.first.typeId() == cached_itype_id ) {
+                        item_location new_loc( vehicle_cursor( vp->vehicle(), vp->part_index() ), &some_item.first );
+                        h.loc = new_loc;
+                    }
+                }
+            }
+        }
+    }
     if( !h.loc ) {
         p.add_msg_if_player( _( "You can't find the heater any more." ) );
         act.set_to_null();

Note I think we may be making a copy here? I don't have a lot of time/willpower to look into this, but if you're interested. for( std::pair<item, int> some_item : tools_map ) {

Thx for your thinking, this problem is solved in my PR #74313.
instead involve the pseudo_tool mess, i choose to consume energy from vehicle, rebuild vp in action from a hack push_back from use_vehicle_tool.

@yuganxia
Copy link
Contributor

yuganxia commented Jun 7, 2024

Thank you again.
#74313
Jobs done, you can close this PR now.

@ShnitzelX2 ShnitzelX2 closed this Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Appliance/Power Grid Anything to do with appliances and power grid astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions <Bugfix> This is a fix for a bug (or closes open issue) [C++] Changes (can be) made in C++. Previously named `Code` [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants