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

Misc fixes for explosions and radio activation #1326

Merged
merged 15 commits into from
Feb 11, 2022

Conversation

olanti-p
Copy link
Contributor

@olanti-p olanti-p commented Feb 9, 2022

Purpose of change

  1. Fix explosions within vehicles causing segfault (fixes Crash when mininuke-ing inside of APC #175)
  2. Fix active explosives within monsters not exploding (e.g. when technician pulls an active grenade)
  3. Fix radio activation mod having infinite uses
  4. Fix items with RADIO_ACTIVATION flag not actually being activated (fixes radio activation mod problem #1276)
  5. Fix active mininuke timer getting reset to 0 on save/load
  6. Fix resonance cascade doing nothing
  7. Allow RC control to choose which rc car to control, instead of selecting semi-randomly
  8. Allow RC control signal to activate items within vehicles or creatures' inventories
  9. Allow RC control signal to activate items on all z-levels, remove max range of 30
  10. Make radio activation mod easier to craft by allowing more battery variants as components

Describe the solution

  1. Defer explosions, to avoid modifying data while iterating. This also makes it possible to explode items within monsters inventories. Idea from game: defer explosions CleverRaven/Cataclysm-DDA#45693
  2. Add code for processing items within monster's inventories
  3. Revert item type to TOOL
  4. Invoke iuse function if item has RADIO_ACTIVATION flag, make RADIO_INVOKE_PROC flag set charges of resulting item to 0, so that the next iuse (e.g. explosion) will be invoked when item is processed on next turn.
  5. Add arbitrary max_charges to the mininuke
  6. Fix iterating over area. It seems that the code has not been updated since Whales wrote it, as it still assumes the reality bubble is 3x3 submaps large.
  7. Get rid of spaghetti and instead iterate over current z-level looking for items with RADIO_CONTROLLED flag
  8. Visit all items on map, in vehicles, in monsters and in Characters and check for relevant flags
  9. Iterate over all tiles within all active z-levels
  10. Add more battery variants as crafting components (cherry-picked from Radio activation mod improvements CleverRaven/Cataclysm-DDA#54213)

Testing

Mostly for the exact scenarios described in the bugreports and bug descriptions.

Additional context

Explosions seem to semi-reliably produce vehicles that have 0 as their cached mass, which results in division by zero and debugmsg informing player of "vehicle with negative drag".

@Night-Pryanik
Copy link
Contributor

You could also look through CleverRaven/Cataclysm-DDA#54213, CleverRaven/Cataclysm-DDA#45661, CleverRaven/Cataclysm-DDA#47066, or CleverRaven/Cataclysm-DDA#47192 to see if there's anything you could salvage.

olanti-p and others added 2 commits February 9, 2022 19:03
- Switch explosion_queue to use std::deque
- Remove limit on radio activation range
- Activate items on all z-levels
- Fix GCC build
- Satisfy tidy
(cherry picked from commit 581e87c190d93ccb30dc301286f84603dc7acdee)
@olanti-p
Copy link
Contributor Author

olanti-p commented Feb 9, 2022

You could also look through ... to see if there's anything you could salvage.

Thanks for the tip!

CleverRaven/Cataclysm-DDA#54213

Cherry-picked crafting changes, the rest I don't think applies.
Unlimited signal reach sounds fun though, I removed max range and z-level restrictions, let's see how it plays out.

CleverRaven/Cataclysm-DDA#45661

Already covered by this PR.

CleverRaven/Cataclysm-DDA#47066

Don't know what to think about this, I kinda like how technicians "swallow" items and force you to kill them.
Out of scope of this PR anyway.

CleverRaven/Cataclysm-DDA#47192

Also out of scope, but looks neat, will throw on the to-do list.

@Coolthulhu Coolthulhu self-assigned this Feb 10, 2022
@@ -807,6 +859,37 @@ float blast_radius_from_legacy( int power, float distance_factor )
( std::log( 0.75f ) / std::log( distance_factor ) );
}

explosion_queue &get_explosion_queue()
{
static explosion_queue singleton;
Copy link
Member

Choose a reason for hiding this comment

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

Would be nicer if it was in game or some other tracker instead of the anti-pattern thing.
Now, game is a god class, but the number of anti-patterns would remain the same instead of being +1'd.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, game would've been a better place for this, but that haven't crossed my mind back then.

process_item_valptr( armor_item, *this );
process_item_valptr( tack_item, *this );
process_item_valptr( tied_item, *this );
}
Copy link
Member

Choose a reason for hiding this comment

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

Why is it not just using visit to go through all the items?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Item processing can result in item being destroyed. visit_items does not allow erasing while visiting, and remove_items_with does not allow modifying (processing) items while visiting and optionally erasing.

It'd either require storing item references, and then erasing them in a separate pass while keeping track of erasure order (because inv is std::vector), or extending visitable with a new method and implementing it for all visitable classes, and this PR already felt overbloated.

Flashbang,
ResonanceCascade,
Shockwave
};
Copy link
Member

Choose a reason for hiding this comment

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

Not snake_cased

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Argh, I did it again

@Coolthulhu Coolthulhu merged commit 7e53d01 into cataclysmbnteam:upload Feb 11, 2022
@Coolthulhu
Copy link
Member

All the issues I have with it are minor enough. It would be nice if they were fixed, but there's no rush.
Our game is a de-facto singleton, so extracting the queue into it would not help much right now, as we don't have "game-less" tests up yet.

@olanti-p olanti-p deleted the misc-explosion-fixes branch February 13, 2022 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

radio activation mod problem Crash when mininuke-ing inside of APC
4 participants