Skip to content

Allow countermeasures to decoy missiles once only#415

Closed
niffiwan wants to merge 1 commit into
scp-fs2open:masterfrom
niffiwan:new_cm
Closed

Allow countermeasures to decoy missiles once only#415
niffiwan wants to merge 1 commit into
scp-fs2open:masterfrom
niffiwan:new_cm

Conversation

@niffiwan

@niffiwan niffiwan commented Nov 4, 2015

Copy link
Copy Markdown
Member

Adds ai_profiles flag
$countermeasures decoy a missile once:
Also (re)adds logging to make it easier to figure out what the
countermeasures are doing. Try adding "+countermeasures" to your
debug_filter.cfg file.

(I'll look at these two in future PRs:

  1. missiles detonating when they lose aspect lock
  2. customising countermeasures missile detonation distance)

Adds ai_profiles flag
    $countermeasures decoy a missile once:
Also (re)adds logging to make it easier to figure out what the
countermeasures are doing. Try adding "+countermeasures" to your
debug_filter.cfg file.
@Goober5000

Copy link
Copy Markdown
Contributor

Why not use a set for cmeasure_ignore_list?

@niffiwan

niffiwan commented Nov 4, 2015

Copy link
Copy Markdown
Member Author

I chose a vector because it's guaranteed contiguous memory which really helps with CPU cache hits on modern CPUs. Regarding a set, I'm don't believe we need the data sorted so that'd point to an unordered_set which should be faster than a set, and unordered_sets have this bucket interface thing which I'm led to believe is a terrible idea that leads to all implementations using linked lists and they're really bad with CPU cache hits.

@asarium

asarium commented Nov 4, 2015

Copy link
Copy Markdown
Member

Map vs vector depends on the size of the vector. If it's small then it's likely that using a vector is faster than using an unordered map. If there are a lot of values then the linear complexity could lead to worse performance than using an unodered map because that typically only needs constant time to determine if a value is already in the map.

@niffiwan

niffiwan commented Nov 4, 2015

Copy link
Copy Markdown
Member Author

the lookup time for a cache miss can be 100x times worse than a cache hit. So can we extrapolate that a vector with up to 100 entries would be on average faster than a map? Is a map likely to have only one extra cache miss vs the vector? Or will there be multiple misses as the internal linked list is followed?

@MageKing17

Copy link
Copy Markdown
Member

Even if an unordered map were a good idea here, std::unordered_map is not a high-performance implementation. For our purposes, a vector is almost certainly the best option; if someone wants to try profiling it in comparison to, say, dense_hash_map, that would be interesting to see... but ultimately a lot of effort for probably not much gain.

@asarium

asarium commented Nov 4, 2015

Copy link
Copy Markdown
Member

How many entries can there be in the vector? I'd guess that the vector would be faster until at least a thousand entries. How many weapons can there be in the game?
I don't think we'll ever hit that cap so it's likely that the vector will work better. At a later point we could try profiling std::unordered_map against the dense_hash_map.

@niffiwan

niffiwan commented Nov 4, 2015

Copy link
Copy Markdown
Member Author

master:code/globalincs/globals.h:#define MAX_WEAPONS 2000

@niffiwan niffiwan added the enhancement A new feature or upgrade of an existing feature to add additional functionality. label Nov 9, 2015
@Goober5000

Copy link
Copy Markdown
Contributor

I've had a chance to review the patch, and it seems legit -- it does appear to work properly as coded. But it also uses a sequence of cascading if() checks that aren't terribly clear, at least in patch form. Furthermore, the cmeasure_ignore_list will contain not only countermeasures that the missile has encountered and is ignoring, but also countermeasures that the missile has tried to chase.

I wonder if the patch could be rewritten to make the countermeasure code more elegant, or if this is the best we can reasonably expect without major surgery in this part of the code...

@niffiwan

Copy link
Copy Markdown
Member Author

Furthermore, the cmeasure_ignore_list will contain not only countermeasures that the missile has encountered and is ignoring, but also countermeasures that the missile has tried to chase.

If I'm understanding you correctly, this is intentional. Each CM should consider each missile only once.

However if you instead mean that a missile will ignore a CM that its chasing, then that's a problem I'll have to look at.

But it also uses a sequence of cascading if() checks that aren't terribly clear, at least in patch form.

That's partly from wanting to keep the retail code exactly as is, just controlled by if() checks. Would extra comments help?

@MageKing17

Copy link
Copy Markdown
Member

I think his point was that the name of the variable doesn't quite match its function, but it does; it ignores that CM for further checks. The fact that we're using two different meanings of "ignore" is potentially confusing, though.

@Goober5000

Copy link
Copy Markdown
Contributor

Yeah, the ignore_objnum field uses one sense of "ignore", and ignore_list uses the other sense. (That's why I suggested "handle_list" in the SCP internal.) I like the sense that ignore_list is using -- it considers each countermeasure once, decides to either ignore it or chase it, and then never considers it again.

I would like to be able to just toss out the existing countermeasure checks, including both the cmeasure_ignore_objnum and cmeasure_chase_objnum fields, and just write a clean function where the retail behavior is a special case of the FSO behavior. In this code, we'd limit the ignore_list to a size of 1 (i.e. we'd only remember the one most recent countermeasure) for retail behavior. The problem is that retail isn't exactly a memory of 1; it also has that weird potential to oscillate between two objects.

Actually, I wonder if that's really a showstopper considering how many other flaws we've found in the countermeasure code. The missile should never have a chance to oscillate if there is only a two-frame limit under retail behavior.

I value very highly the preservation of retail code when appropriate, but for countermeasures it may not be meaningful.

@niffiwan

Copy link
Copy Markdown
Member Author

If we limit the size of ignore_list to 1, then missiles should still be able to "oscillate" for two frames. I'll try a new PR based on that idea, then we can rip it apart (aka rigorously assess/test it) to ensure it'll match (or very closely approximate) retail behaviour.

@niffiwan

Copy link
Copy Markdown
Member Author

See #463 for the alternative solution discussed, I've thought through the logic several times and am reasonably confident that this will act the same as retail. BUT, I'm super keen to see that confirmed/challenged.

@niffiwan

Copy link
Copy Markdown
Member Author

alternate solution implemented in #463

@niffiwan niffiwan closed this Jan 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement A new feature or upgrade of an existing feature to add additional functionality.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants