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

Fixes for active transforming ter/furn, add grid-powered space heaters #1204

Merged

Conversation

chaosvolt
Copy link
Member

@chaosvolt chaosvolt commented Dec 26, 2021

Summary

SUMMARY: Content "Fix charge_watcher transformation skipping important functions, add grid-powered space heaters"

Purpose of change

Per my musings while messing with vehicle heaters, I realized we now have what's needed to allow for installing grid heaters. Well, after a while of leaving this PR on hold as I didn't realize why heaters weren't emitting heat, @KheirFerrum and I both started looking through functions and found out the relevant function was skipping furn_set in favor of set_furn.

Describe the solution

  1. Updated grid_furn_transform_queue::apply in distribution_grid.cpp to correctly feed the needed info to furn_set instead of set_furn, so it won't skip out on important functions like updating emissions. Also makes use of a tripoint Kheir spotted that was exactly what I needed to get it working, which was previously buried in an if block.
  2. Added furniture definitions for installed space heaters. Like with the floor lamp they toggle on and off, and the on states use charge_watcher and steady_consumer. When active they consume the same power that their item counterparts are set to and produce the same emission.
  3. Added construction recipes for wiring up both heater variants.

Describe alternatives you've considered

Rigging up constructable air conditioners could be added too, maybe?

Testing

  1. Checked affected files for syntax and lint errors.
  2. Compiled and load-tested.
  3. Constructed a space heater and turned it on.
  4. Confirmed it correctly converts to the active version when it warms up, and emits hot air.
  5. Turned the heater off, observed that the hot air was decaying.

Additional context

Tile symbol and color evidently aren't carried across by copy-from, nice.

@Coolthulhu Coolthulhu self-assigned this Dec 27, 2021
@Coolthulhu
Copy link
Member

Coolthulhu commented Dec 28, 2021

Turned it on, confirmed it emits hot air 2.

Doesn't happen until you reload the map with an active heater in it.
This is because the "grid furniture turn on/off" function uses submap::furn_set, not map::furn_set. It can't always use map::furn_set because it can operate on objects outside map.

It could check map bounds and specifically call map's version for those, but it doesn't do that at the moment.
If it did use map version, it would need a fix for this bit:

    if( new_t.has_flag( "EMITTER" ) ) {
        field_furn_locs.push_back( p );
    }

It unconditionally adds the new point to the vector, regardless of whether old_t was an emitter or not, and doesn't remove the point from the set if old_t was an emitter and new_t isn't. Since field_furn_locs is a vector and not a set, it can contain duplicates and those will be processed multiple times, leading to multiple emissions after heater would turn off and on a few times.
But again: this bug doesn't happen, because currently, the heater only works if the map is loaded while the heater is active and powered.

@chaosvolt
Copy link
Member Author

Oh, so when I saved the game and tested editing the JSON, that's what fixed it, not the JSON edit itself? Well crap, that means the feature won't actually work right as intended.

I'll go ahead and close this for now, since not sure how easy it'd be to fix.

@chaosvolt chaosvolt closed this Dec 28, 2021
@chaosvolt
Copy link
Member Author

So after some source-diving to look into what makes this tick, it took some talking with Kheir on the discord and giving this PR a second look to realize I missed something absolutely critical in my JSON: the EMITTER flag.

Time to reopen, get this fixed up, then give it a second test.

@chaosvolt chaosvolt reopened this Nov 14, 2022
@github-actions github-actions bot added the JSON related to game datas in JSON format. label Nov 14, 2022
@chaosvolt
Copy link
Member Author

Wait no, the active version DOES have the correct flag. I'm still going to look at this but we'll see if I get results or not.

@chaosvolt
Copy link
Member Author

Found the cause: grid_furn_transform_queue::apply skips past furn_set (which includes fun stuff like updating the emitter) and goes to calling set_furn (what the former calls at the very end after doing other stuff).

@chaosvolt
Copy link
Member Author

chaosvolt commented Nov 14, 2022

I need to figure out how to make grid_furn_transform_queue::apply give a call to furn_set instead of skipping directly to set_furn:

        tripoint_abs_sm p_abs_sm;
        point_sm_ms p_within_sm;
        std::tie( p_abs_sm, p_within_sm ) = project_remain<coords::sm>( qt.p );

        submap *sm = mb.lookup_submap( p_abs_sm );
        if( sm == nullptr ) {
            return;
        }

        const furn_t &old_t = sm->get_furn( p_within_sm.raw() ).obj();
        const furn_t &new_t = qt.id.obj();

        sm->set_furn( p_within_sm.raw(), qt.id );

I figured out I need to replace sm->set_furn with m.furn_set, but what I can't puzzle out is what it will want for a tripoint in place of p_within_sm.raw().

EDIT: What seems to be exactly what I'm after is already defined later in the function, just buried in an if statement. Will test in a bit.

@github-actions github-actions bot added the src changes related to source code. label Nov 14, 2022
@chaosvolt chaosvolt marked this pull request as draft November 14, 2022 06:23
@chaosvolt chaosvolt changed the title Add grid-powered space heaters Fixes for active transforming ter/furn, add grid-powered space heaters Nov 14, 2022
@chaosvolt chaosvolt marked this pull request as ready for review November 14, 2022 06:53
@Coolthulhu Coolthulhu merged commit 21d1ecc into cataclysmbnteam:upload Nov 19, 2022
@Coolthulhu
Copy link
Member

Couldn't get it to break, so it's probably working. It's a bit late to demand proper test cases, I'll do it next time.

@chaosvolt
Copy link
Member Author

Alright. I'm still worried about the test failure you mentioned in #2187, since this is the PR that changes the code likely to cause it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JSON related to game datas in JSON format. src changes related to source code.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants