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

Update stat filters #1060

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

nqrcqn
Copy link
Contributor

@nqrcqn nqrcqn commented Sep 5, 2019

Filters now also support fractured, crafted and enchanted mods. Added new strings and some new filters.

Filters now also look for fractured, crafted and enchanted mods. Added new strings and some new filters
POEApi.Model/Gear.cs Outdated Show resolved Hide resolved
@nqrcqn
Copy link
Contributor Author

nqrcqn commented Sep 6, 2019

Talisman filter doesn't return the Talismans in my stash, also the Craftables filter returns very few results, may be fixed also.

Edit: Talisman filter doesn't work because they get the Amulet base type here but changing the order there removes them from amulets and adds to Talismans. https://github.com/Stickymaddness/Procurement/blob/master/POEApi.Model/GearType/GearTypeFactory.cs

@nqrcqn
Copy link
Contributor Author

nqrcqn commented Sep 7, 2019

To add new strings, converted IncreasedPhysicalDamageFilter and ManaRegenFilter to use OrStatFilter

Can I add implicit mods to ExplicitModBase or was the goal there excluding implicit mods?

@@ -16,7 +16,7 @@ public string Keyword
{
get
{
return "Full Bestiary Orb";
return "Captured Beasts";
Copy link
Contributor

Choose a reason for hiding this comment

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

"Captured" beasts include those not just stored in orbs. I used the name "Full Bestiary Orb" because that's essentially what GGG calls them -- see the file name used for the icon: BestiaryOrbFull.png.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used captured beasts because it's the same term used on trade, the other sounds more like an internal name

@@ -14,6 +14,15 @@ class AmuletFilter : GearTypeFilter
public AmuletFilter()
: base(GearType.Amulet, "Amulets")
{ }
public override bool Applicable(Item item)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a blank line before this one.

public override FilterGroup Group
{
get { return FilterGroup.Default; }
}
public ManaRegenFilter()
: base("Mana regen", "Items with increased Mana Regeneration Rate", "increased Mana Regeneration Rate", "Mana per second", "Mana Regenerated per second", "as extra Mana Regeneration")
Copy link
Contributor

Choose a reason for hiding this comment

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

Split across multiple lines.

if (mod.Contains(keyword))
return true;

if (gear.EnchantMods != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, enchant mods are not explicit mods (and you can argue crafted and fractured mods aren't either). However, I think the intent of this class (as it is being used) is to search the mods of an item. As such, I could see also adding implicit mods to this class, if we rename the class.

Ideally, which mods are search should be a parameter to the value/property being searched for and can be customized by the user. But that is a much larger change that is outside the scope of this PR.

Copy link
Contributor Author

@nqrcqn nqrcqn Oct 6, 2019

Choose a reason for hiding this comment

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

If there is interest, it will be easy to support customized searches on search box with my other PR, like name: type: mod: explicit: implicit: crafted: fractured: enchanted: flavour: property: requirement: description: prophecy: card:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to be added with only explicit mods in mind, so if we're going to add implicit mods, I think it's better to remove the file and use StatFilter instead

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.

2 participants