RAH v3 #689
RAH v3 #689
Conversation
New Reactive Armor Hardener code that should work correctly.
|
Well, it doesn't "correctly" handle uniform damage types. (Correctly in quotes because uniform damage would cause the RAH to adjust, but we need to handle it as a "default" scenario.) For uniform damage type, it should't be in the loop at all. You also adjust the damage down twice by resists, once initially and once on your first loop, before the RAH adjusts (notice Damage taken this cycle has been adjusted down again): Maybe @ccplarrikin can confirm/deny this sorting order. We currently don't cover it at all, but it's a pretty deep edge case unless someone goes hunting for it. It's much more of an issue with a cycling RAH vs a static one as it will be far more likely to hit a "tie" scenario. The other issue with this is responsiveness. I haven't done profiling on this, but it's adding a significant amount of time to the process. It's made worse when initially opening a ship, as the effects files are run through three times instead of once. Add on the fact that effect files are hit for each booster, and worst case scenario it appears to be adding a several second delay. |
|
In what situations is it not correctly handling uniform damage? I did a bunch of tests shooting various ships with a Gecko and it gives the correct results for all them. I don't understand your "default scenario" idea. The damage is adjusted multiple times. It first gets adjusted for the ship's resistances with all the other mods except the RAH, then each cycle it multiples that by the RAH profile to get the actual incoming damage (ignoring stacking penalties with Damage Controls). As for the several second delay, something seems wrong there. In the log snippet you showed it says ERROR instead of DEBUG and %d instead of the cycle number. Did you tweak the debug logging? That's probably the performance issue, and in hindsight I should've disabled the logging code for performance reasons. |
In the scenario of Basically, if the damage profile is 25/25/25/25 we need to reset it back. Uniform damage doesn't necessarily mean we want the RAH to adjust, it could mean that we want to see what it looks like before it adjusts. We have to be able to handle that scenario, and traditionally we've done it with the uniform profile. So you are absolutely correct that the RAH will handle uniform resists. The problem is that we want an exception to the rule, and for it to not handle it. We could look at putting some sort of toggle switch on it (like we do with reload timers), but that adds a lot of complexity.
And that's part of the problem. Unless you recreate the entire fitting engine (which I don't think you can do in just a few lines), it's not going to handle stacking properly.
I switched from debug to error because it was faster to do that than change my command line. :) As for You don't have a number to insert.
It's not the logging. I mean, sure, it doesn't help any, but it's not the primary cause. As an example, fit that has a RAH with 3 boosting ships each with a RAH. Pyfa is freshly launched, so no extra lag from have 20 fit windows open. v2 v3 Since both have logging enabled (though, granted, v3 has a couple more lines than v2), that's not much of a factor. But yeah, the looping adds about 2 seconds to the initial fit open time. Subsequent fit opens are better, though still delayed. Since Pyfa already has performance issues, this is a pretty big hit. It's a bit of an edge case (not too many people will have 4 fits all using RAH), but it won't be unheard of in armor fleets either. |
This makes sense as there really isn't anything in the game that does complete omni damage. I think users who use a uniform damage profile will want to see an average scenario. However, it's technically an incorrect calculation - we are knowingly introducing a bug that inevitably will be reported by someone. It may be possible to have some sort of setting (such as factor reload time into calculations) where we can have the RAH adjust or not adjust. Thoughts? Otherwise this is great stuff. Still haven't had a chance to dig into it yet though edit: I got to the point quoted and left a comment. Literally the next line down:
🤦 However I don't think it'll add a lot of complexity. It's a setting in |
Oops, I accidentally deleted the end of that line when deleting the next line. After this discussion it's apparent I need to fix a couple things. I'll close this pull request and put in a new one once they're fixed. |
|
You don't have to close this one. Just add commits to the branch you used to create this pull request and push :D |
|
Ok, I commented out the logging and made a couple minor tweaks. On my system (famous last words) the RAH calculations reliably run in a millisecond, so there shouldn't be any noticeable performance issues. Here's the log of loading a fit with a RAH, three different boosters with their own RAHs, and two projected fits with RAHs: |
Again, it's not the logging that's the cause. The RAH is complex enough that it's well worth adding quite a bit of debug logging. It's a bit faster, but still clocks in at almost a second and a half. So improvement in the right direction. |
|
What exactly is taking a second and a half? Is it loading a particular fit, toggling the RAH, or something else? If you use the same fit but take the RAH off the ship and any boosters/projectors, how long does it take? Could you go in debug mode (-d in the arguments when launching) and see how long it takes to do the various steps of calculating the fit, like I pasted above?
It's interesting that took 64ms. It would need to get 9 attributes between those lines, which seems very slow the first time (and slow in general since it's a dictionary lookup with a string key). I believe your code also needs to get the same data (or hard code things that could change in a patch...). Try putting a logging line right before the loop and one right after it. The loop part should be very fast, with the getting and setting attributes part being slowish. As for the "Loop found: 11-11" bit, the numbers are probably wrong since I commented that out before tweaking things that affect the indices it uses. |
Initial load of a fit. Profiling wise, I'm comparing it to the times for v2. (v1 runs slightly slower than v2, but they're both close enough to be negligible.) Code is otherwise the same (each is a branch off Pyfa-Master with only the one file changed). I'll see about running in debug mode for additional logging, I actually want to go through and add debug logging through out the application so we can better profile/troubleshoot issues like this one. I copy/pasted the wrong lines. The two posted above are for one run through of the RAH effect. Here's the first to final debug line. Profiling wise, at the end of the first time the adaptivearmorhardener effect is hit there is a very odd delay: That's very nearly a half second delay right there. Here's the full log dump if you're curious: |
So getting the attributes and simulating the RAH cycles took 64ms the first time then 13ms the next two. That's several times longer than it takes for me, but it still only accounts for 15% of the total time. isn't taking that much time. Though it does seem those lines could be made a bit more efficient. |
Which goes back to what I said to on the issue thread:
There's something in here that's causing it to take much longer (3-4 times as long) than the v1 or v2 versions, at least for a few particular fits. I don't have the cycles at the moment to troubleshoot what that is exactly, as it's a bigger issue than this one effect file. A holistic effort to enable debugging across Pyfa is on my short list of things to do, and at least part of this will go away if/when CCP kills fleet boosts (since the booster also gets hit by the RAH effect code, though interestingly enough it doesn't fire for projected fits). But either of those isn't in the very near future. |
Assuming the v2 you referred to is what you have up on your pull request, there should be no difference except what happens between the logged cycle start and end times, and the RAH resistances when it's done. The effect body can be ruled out as the source of the long delay since it happens between the end and beginning of the log entries, so that leaves whatever code uses the RAH resistances (or something that runs the code without logging, which I doubt exists). Is there perhaps a bug in the code that displays the RAH stats in the UI? I wish I could replicate your performance problems. I don't see any difference in timing between the three versions. |
Correct.
Well, I don't think we can rule anything out yet. Clearly it's triggered by something in the RAH code, even if it's external to that code block.
Most likely it's something along the lines of the rest of the fit recalculating from the RAH changes. But without profiling, it's impossible to say what.
Bugs? What bugs? I only see features. |
Since the RAH code is on "late" runtime, wouldn't that all be in the line
which is the same between our versions? It seems like everything else would just take the modified ship attributes after that, not caring about such differences. |
|
vOv Your guess is as good as mine at this point.
Late refers to when it's run in the whole fitting engine cycle. There's early (start of cycle) and late (end of cycle). Initially it was early, which means that when the effects file for There is still a question if fleet boosts will affect it, since they are also a late runtime. Basically it's a toss up if anything that's flagged as late will come before or after the RAH. |
This still works the same, but I think I changed a couple things that didn't look right.
|
Well I've confirmed with my trusty Megathron Navy Issue and Depleted Uranium test (and the other simulator) that the stacking penalties with damage controls can indeed make the result inaccurate. It wouldn't be hard to compensate for that, but it would involve modifying eos.ModifiedAttributeDict to expose the existing stacking-penalized effects so the RAH effect could see if there's anything stacking with it. I'm not sure how wise it is to go doing such things, so I'll leave that alone. |
Yeah. It doesn't actually run the fitting engine to recalculate the fit. I don't know if you can set the values and tell it to recalc without leaving the loop. I have a feeling that will recursively run through the effects. This is probably something we just simply have to accept as something that will be off for the RAH. |
|
It doesn't need to rerun the whole fit calculation, it just needs to find out if there's anything else stacking with it and compensate for that part of the calculation. It's just getting that information that seems a little questionable since the most reasonable way would involve tweaking the effect manager to expose more information that doesn't really have use except in this case. |
|
You are assuming that there is nothing else that changes it. This may or may not be true currently, and definitely could change as CCP adds modules/effects. There's not much we can do about it though, kicking off an entire refit calc would be a brutal force method, but I don't see another way to guarantee that there's no stacking issue or other modules/effects coming into play. |
|
Yep. Just looking at solutions to current issues, and this one is quite doable but starting to get a bit extreme for what it does. |
Can I get a summary of the problem here and your ideas on fixing it? As I understand it, we have an issue with stacking with damage control? |
The problem is that when a ship has a damage control and an RAH the resistances in the RAH cycle simulation are a bit off due to not accounting for stacking penalties. In some cases that changes the outcome. with Depleted Uranium ammo, the RAH cycles between 0/0/34.5/25.5, 0/3/37.5/19.5, 0/0/39/21, and 0/3/33/24. Add a DC2 and it stops at 0/0/34.5/25.5 because the stacking penalties make the explosive resistance slightly lower relative to thermal. That results in the RAH profile being 0/0/34.5/25.5 instead of effectively 0/1.5/36/22.5. It's a tiny difference on that fit, but on others it could potentially mean the difference between say, stopping at 0/30/30/0 or going to 0/48/9/3. The easiest and most efficient way to fix it would be to check if the ship has a damage control (or for forward compatibility, anything in the same penalty group) and account for the varying stacking penalties in each cycle of the loop. Seeing as eos is apparently a separate project and such an ability would be of limited use, I'm not sure how reasonable that would be to do. |
It's not really a separate project. It used to be, but then it was merged into pyfa proper years ago. There's another So basically we have the reactive armor hardener stacking with damage control and we don't want that... In that case (and I haven't tried this), couldn't we just specify a special-case stacking group for the RAH? Right now it's set as If that doesn't work, then feel free to add the changes to need to in EDIT: penalty group is found on this line: https://github.com/pyfa-org/Pyfa/pull/689/files#diff-189776af73b44511f6de68f6b32214faR109 Changing it to |
No. We do want it to stack. The problem is that we we need to recalculate the stacking penalty every loop, which we don't do now. The easiest solution would be to set a global variable to flag that we're inside the adaptivearmorhardener and skip it so we don't get in an infinite loop, and fully recalculate the fit each loop. This would be what EVE does essentially, but EVE can do it every 5 seconds while we're trying to do it instantly. Perhaps a further wrinkle is that CCP is adding more modules in that line. They added a selectable damage DCU, and a DCU that sets resists to 99%. So who knows what else that will impact. |
|
k. Let me begin by stating congrats to both of you for having contributed some very good data and discussion on this topic. Hopefully anyone who does a google search for After reading and re-reading all the discussions regarding the RAH and code review, these are the conclusions I have come up with:
That being said, I've decided to merge this PR. It was pretty difficult to decide as they are both good solutions to a complex problem. It really came down to two things for me (keeping in mind this is a simulation and thus accuracy is never going to be guaranteed): the resist loop, and (to a much lesser extent) performance.
One difference between the two is how to handle Uniform damage. I tend to lean towards @Ebag333's method of ignoring it as we might want to show what it's like at it's unmodified default, but I can also see the other side in that we might want to know how it would handle IRL... I keep flipping between which route to go. For now, I think it's fine to leave it as-is and maybe tweak it if needs to be (like, we get a lot of folks who are expecting it a certain way). And with that, I hope to put RAH stuff behind me xD |
New Reactive Armor Hardener code that should work correctly and give an accurate result. Based heavily on @Ebag333's file.
Refer to issue #680 for details.