Weapon attack cleanup #5922
Weapon attack cleanup #5922
Conversation
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
a4d6fee
to
6902929
Compare
|
This has a ton of data stuff attached to it. I'm assuming you didn't update your branch to master before doing the PR. |
Mistakes were made. I'm working on it. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
5 times, most recently
from
ceec3db
to
262c4ca
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
2 times, most recently
from
d7d9074
to
fd5dc0e
Compare
WeaponMounted.getBayWeapons() iterates through the entire bay before returning, which isn't always necessary. It has been refactored to allow for the stream to be used directly.
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
2 times, most recently
from
9f6bd6e
to
175a3f1
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5922 +/- ##
============================================
+ Coverage 28.90% 28.92% +0.01%
- Complexity 13926 13943 +17
============================================
Files 2539 2539
Lines 268363 268319 -44
Branches 47933 47872 -61
============================================
+ Hits 77578 77606 +28
+ Misses 186817 186748 -69
+ Partials 3968 3965 -3 ☔ View full report in Codecov by Sentry. |
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
6 times, most recently
from
f1df6d9
to
f168263
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
f168263
to
22eef5c
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
22eef5c
to
29643a5
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
8132fd6
to
34ecf28
Compare
| } | ||
| } | ||
| if (!usable) { | ||
| if (!weapon.streamBayWeapons() |
There was a problem hiding this comment.
Request to keep streams out of if statements. I'd rather have an extra line and boolean assignment and a variable name that says what this test is about.
There was a problem hiding this comment.
I don't see how having a distinct boolean would add clarity beyond what the existing comment provides.
There was a problem hiding this comment.
Do you want me to change the comment to be an initialized boolean anyway?
There was a problem hiding this comment.
I agree with @SJuliez. It makes the code harder to read.
| spotter = game.getEntity(ti.attackerId); | ||
| } | ||
| } | ||
| final Targetable currentTarget = target; // Required for concurrency reasons |
There was a problem hiding this comment.
For readability and code length, this does not feel like an improvement to me.
There was a problem hiding this comment.
target really should be final, but the current implementation for Swarm LRMs prevents that.
There was a problem hiding this comment.
I find this more readable because it makes it much easier to tell that it is simply getting any entity that successfully applied TAG to the target.
There was a problem hiding this comment.
If this is an issue, I could just drop this commit. I'd rather not hold up further review of this pull request due to a few controversial changes.
There was a problem hiding this comment.
This changes the code and may have other impacts as the old code picked the last target that matched where as the new code picks a random one.
Depending upon the rules, this may not be a valid change.
There was a problem hiding this comment.
This will literally never make a difference. The source of the TAG does not matter.
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
5 times, most recently
from
4df8c8c
to
e2bacbf
Compare
HammerGS
added
For New Dev Cycle
This section of code specifically checks whether a BA squad has already fired a taser or Narc at a different target.
It may be possible to optimize this further if it can be assumed that all CI attacks either have the infantry flag or are field guns, but I've settled for replicating the previous logic for now.
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
e066406
to
bdfb73b
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
0e3fc44
to
d16dce5
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
332a3fd
to
891a17d
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
2 times, most recently
from
e6131e9
to
5b4ce3f
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
2 times, most recently
from
3b96361
to
33a1c1f
Compare
Saklad5
force-pushed
the
weapon-attack-cleanup
branch
from
33a1c1f
to
014cb8f
Compare
|
First I want to say thank you for the PR and you're interest to improve MegaMek. However, we have some concerns about this refactor, particularly given its scope and the core area it affects. Our main points of concern are: Readability: We strive to keep our code easy to understand. While streams and optionals are useful, overusing them can make the code less accessible, especially for newcomers or those with different skill levels. I'm not a coder but years of working with the code I've learned to read it, but I don't fully understand what I'm looking it here. Running it with the debugger hasn't helped me. Consistency: MegaMek's code has developed over many years, establishing its own style and conventions. This creates a scenario where we need to refactor and roll out changes in a consistent pattern, and behind the scenes discussions around this happen and having refactoring PR's can disrupt plans. Performance: Although it's not our top priority in most areas, unnecessary use of streams and optionals can slow things down. We prefer changes that offer clear performance benefits. Risk management: Refactoring critical parts of the code can lead to unexpected issues. MegaMek is complex, so we need to carefully consider and justify changes. Smaller, focused updates are easier to review and less likely to cause problems. Collaboration: We value feedback and teamwork in our development process. If concerns raised by Devs (or the tools) aren't addressed, it's unlikely we'll accept a pull request. (You can find our guidelines for developer and contributor expectations on the MegaMek Wiki.) Likewise, after discussions with Devs and getting their feedback if that feedback is ignored it does reduce the likelyhood of a PR being accepted. Given these concerns, we're going to hold off on merging this PR while we evaluate its overall impact. Please address the existing comments on the PR first. Once that's done, we'll discuss the next steps. For future reference, it's a good idea to chat with the Dev team before starting any major refactoring work. We are open to using these but want to start small and in specific areas of the code. As I mentioned in my recent Sarna interview, "You need to earn some street cred by showing you understand our 20-year-old codebase. It needs careful handling, like dealing with a 680-year-old Icarus. You've got to really learn the code, not just know how to code." |
SJuliez
removed
the
For New Dev Cycle
| if (ae.hasC3() || ae.hasC3i() || ae.hasActiveNovaCEWS() | ||
| && game.getEntitiesVector().stream().anyMatch(en -> | ||
| !en.isEnemyOf(ae) | ||
| && en.onSameC3NetworkAs(ae) | ||
| && Compute.canSee(game, en, target))) { |
There was a problem hiding this comment.
Please don't do this inside of a conditional block. If you're going to do a stream, please do so either inside or outside of it.
This is a series of refactors centered around eliminating unnecessary raw loops from WeaponAttackAction.