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

Filter Units Based on Weapon Class #3932

Merged
merged 5 commits into from
Oct 14, 2022
Merged

Filter Units Based on Weapon Class #3932

merged 5 commits into from
Oct 14, 2022

Conversation

Seosaidh
Copy link
Contributor

This branch enables the advanced filter to filter units based on weapon class in addition to just individual weapons/equipment. For example, if you previously wanted to get all mechs that mounted any type of LRM (intro, standard, or advanced), you would have to add a whole plethora of weapons to the filter list. Now you just selected the "LRM" class and add that to the filter.

To implement this feature, I added a list of weapon classes to the MechSummary object, along with corresponding quantity counts for each weapon class the unit mounts. The advanced filter was then updated to include a weapon class table that allows advanced filtering based on weapon class. This also involved modifying the filter tokens (adding a new WeaponClassFT class), the ExpNode class (adding a weaponClass field and a new constructor to fill it in), and the evaluate functions (to take the list of weapon classes the mech mounts, and to compare it correctly with the ExpNode).

Add Weapon Class Counts to MechSummary
Adds a new FT called WeaponClassFT that is adjacent to EquipmentFT.
Additionally adds an Integer member to ExpNode. This is set to null when ExpNode is constructed with a String (only constructor before).
But the new constructor (which takes an Integer instead of a String), sets the weaponClass.
If the ExpNode has a non-null weaponClass, we know it's a request for a filter on weaponClass instead of general equipment.
Logic is then added to filter units based on weapon class (which was added to the MechSummary in a previous commit).
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Base: 23.56% // Head: 23.54% // Decreases project coverage by -0.01% ⚠️

Coverage data is based on head (7f92ce0) compared to base (6c144ef).
Patch coverage: 0.68% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3932      +/-   ##
============================================
- Coverage     23.56%   23.54%   -0.02%     
  Complexity     4810     4810              
============================================
  Files          2212     2212              
  Lines        243034   243165     +131     
  Branches      45481    45506      +25     
============================================
  Hits          57264    57264              
- Misses       184311   184442     +131     
  Partials       1459     1459              
Impacted Files Coverage Δ
.../megamek/client/ui/swing/AdvancedSearchDialog.java 0.00% <0.00%> (ø)
megamek/src/megamek/common/MechSearchFilter.java 0.00% <0.00%> (ø)
megamek/src/megamek/common/MechSummary.java 0.00% <0.00%> (ø)
megamek/src/megamek/common/WeaponType.java 84.14% <100.00%> (ø)
megamek/src/megamek/common/MULParser.java 0.00% <0.00%> (ø)
...c/megamek/client/ui/swing/tooltip/UnitToolTip.java 0.00% <0.00%> (ø)
megamek/src/megamek/common/verifier/TestAero.java 1.66% <0.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

/**
* A table model for displaying weapon types
*/
public class WeaponClassTableModel extends AbstractTableModel {

Check notice

Code scanning / CodeQL

Inner class could be static

WeaponClassTableModel should be made static, since the enclosing instance is not used.
@@ -1294,6 +1461,25 @@
}
}

public class WeaponClassFT extends FilterTokens {

Check notice

Code scanning / CodeQL

Inner class could be static

WeaponClassFT should be made static, since the enclosing instance is not used.
Bail on filter if null equipment or ExpNode encountered.
@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2022

This pull request introduces 2 alerts when merging 381b54a into 1390a08 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2022

This pull request introduces 2 alerts and fixes 1 when merging 6a5c09d into 1390a08 - view on LGTM.com

new alerts:

  • 1 for Useless comparison test
  • 1 for Dereferenced variable may be null

fixed alerts:

  • 1 for Dereferenced variable may be null

@HammerGS
Copy link
Member

We would need the issues identified in the automated code review addressed before we can do a final review.

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 1 alert when merging 852010f into 1390a08 - view on LGTM.com

fixed alerts:

  • 1 for Dereferenced variable may be null

Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

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

A couple minor changes, looks good otherwise.

@Seosaidh
Copy link
Contributor Author

@NickAragua I'm not sure why it won't let me respond to your comment about the exception, but here we are.
I copied the same style as the equipment and weapon tables above it. They also don't check for the exception, probably because the value comes from a drop-down box, and so should always be present. The same is true for this table.

@lgtm-com
Copy link

lgtm-com bot commented Oct 13, 2022

This pull request fixes 1 alert when merging 7f92ce0 into 1390a08 - view on LGTM.com

fixed alerts:

  • 1 for Dereferenced variable may be null

Copy link
Member

@NickAragua NickAragua left a comment

Choose a reason for hiding this comment

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

Looks good. I'll get it merged later tonight.

@NickAragua NickAragua merged commit c101f42 into MegaMek:master Oct 14, 2022
@HammerGS
Copy link
Member

I think there might be an issue with this. I started by setting up a search for Gauss and LRMs.

It returned no results, but if I set up a search with Gauss rifles (Clan and IS) and all the common LRMs(clan and IS) in the normal search it returns over 50 meks.

Going back into the search I get the following. Nulls in the search.

image

@Seosaidh
Copy link
Contributor Author

I think there might be an issue with this. I started by setting up a search for Gauss and LRMs.

It returned no results, but if I set up a search with Gauss rifles (Clan and IS) and all the common LRMs(clan and IS) in the normal search it returns over 50 meks.

Going back into the search I get the following. Nulls in the search.

image

Huh. I didn't test with Gauss. I'll check on my end.

@Seosaidh
Copy link
Contributor Author

Yep. I get the same problem. I'll have a look-see. Incidentally, while filtering on LRMs works, when you close, then re-open the advanced search, you eventually get a (1 null) string, like you're seeing with the Gauss.

@HammerGS
Copy link
Member

Also looks like no matter what you do if you come back into advanced search the null values above are there.

I love this ability to search like this..btw.

@Seosaidh
Copy link
Contributor Author

This PR should be reverted. I made a mistake and was filtering based on the Aero weapon class, not a human understanding of weapon classes. The primary result of this is that Gauss weapons are labeled under AC class, since that is how Aero classifies them.

The feature will have to be implemented via a different method.

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.

3 participants