Filter theme comments causing "Too many arguments" error

[cezelkowitz]

Simple example that causes "Failed to load Item Filter: Line 11: Too many arguments" error when loaded

# Script edited with Filtration - https://github.com/ben-wallis/Filtration

# # Currency

# ## Currency | T0

Show
Class "Currency"
BaseType "Mirror of Kalandra"
SetTextColor 0 255 0 # Currency
SetFontSize 40 # Currency T0
PlayAlertSoundPositional ShMirror 200
MinimapIcon 0 Red Diamond # Currency T0
PlayEffect Red # Currency T0

[azakhi]

I am not getting an error with version 0.20. What version are you using? Also could you check if there is a file named Filtration_errors_DATE_HERE.log in installation folder and share it with us?

[ben-wallis]

I also can't reproduce in 0.20 - although I did find a typo in the metadata file that's used to check for updates so no users would have got the update notification from 0.18/19 > 0.20 (now fixed).

@cezelkowitz can you confirm whether you still get this bug in 0.20?

[cezelkowitz]

I am using version 0.20. Attached is the log file though the error is in game and the app (Filtration) works fine.

Filtration_errors_2018-08-30.log

[GlenCFL]

The SetFontSize rule doesn't allow for trailing text in-game. We should probably be stripping that.

I maintain a cheatsheet for this kind of stuff here.

[azakhi]

Looks like MinimapIcon and PlayEffect also doesn't allow it. CustomAlertSound didn't give an error. Apparently it allows comment lines right after those tho. So we can change regular expressions and write themes on a new line.

[cezelkowitz]

Nice finds guys. Thanks! Here is the full filter in case it's relevant.
MappingFilter.txt

[cezelkowitz]

Might I suggest a cleaner way to handle themes? It's not very intrusive and can fit into your current way of managing this without using extra files (deriving the theme from the filter itself). It's non-intrusive to the DSL of a .filter and uses a common/popular language (JSON).

Here is what you have:

Show
    Class "Currency"
    BaseType "Mirror of Kalandra"
    SetTextColor 0 255 0 # Currency
    SetFontSize 40 # Currency T0
    PlayAlertSoundPositional ShMirror 200
    MinimapIcon 0 Red Diamond # Currency T0
    PlayEffect Red # Currency T0

Here is what I suggest:

# Script edited with Filtration - https://github.com/ben-wallis/Filtration :^)
# Filtration Themes
# {"SetTextColor":{"Currency":"0 255 0"},"SetFontSize":{"Currency T0":"40","Currency T1":"38"},"MinimapIcon":{"Currency T0":"0 Red Diamond","Currency T1":"0 Red Diamond"},"PlayEffect":{"Currency T0":"Red","Currency T1":"0 Red Diamond"}}

Show
    Class "Currency"
    BaseType "Mirror of Kalandra"
    PlayAlertSoundPositional ShMirror 200
    #{"BlockTheme":{"SetTextColor":"Currency","SetFontSize":"Currency T0","MinimapIcon":"Currency T0","PlayEffect":"Currency T0"}, "Enabled":"false"}

Show
    Class "Currency"
    BaseType "Exalted Orb"
    PlayAlertSoundPositional ShExalted 200
    #{"BlockTheme":{"SetTextColor":"Currency","SetFontSize":"Currency T1","MinimapIcon":"Currency T0","PlayEffect":"Currency T0"}, "Enabled":"false"}

1. Store the themes at the top in a JSON map.
2. Store applicable theme attributes under the theme block.
3. Toggle the theme on/off making use of an enabled flag.
4. Apply enabled theme attributes, and ignore disabled theme attributes.

# Script edited with Filtration - https://github.com/ben-wallis/Filtration :^)
# Filtration Themes
# {"SetTextColor":{"Currency":"0 255 0"},"SetFontSize":{"Currency T0":"40","Currency T1":"38"},"MinimapIcon":{"Currency T0":"0 Red Diamond","Currency T1":"0 Red Diamond"},"PlayEffect":{"Currency T0":"Red","Currency T1":"0 Red Diamond"}}

Show
    Class "Currency"
    BaseType "Mirror of Kalandra"
    SetTextColor 0 255 0
    SetFontSize 40
    PlayAlertSoundPositional ShMirror 200
    MinimapIcon 0 Red Diamond
    PlayEffect Red
    #{"BlockTheme":{"SetTextColor":"Currency","SetFontSize":"Currency T0","MinimapIcon":"Currency T0","PlayEffect":"Currency T0"},"Enabled":"true"}

Show
    Class "Currency"
    BaseType "Exalted Orb"
    PlayAlertSoundPositional ShExalted 200
    #{"BlockTheme":{"SetTextColor":"Currency","SetFontSize":"Currency T1","MinimapIcon":"Currency T1","PlayEffect":"Currency T1"},"Enabled":"false"}

[azakhi]

The problem with this kind of solution is some people doesn't like it when Filtration requires its own syntax. Also Filtration should still support commonly used styles and a trailing comment after the rule is probably the most common one.

[ben-wallis]

I'm not sure there's a problem with something like what @cezelkowitz suggested, in the same way that there was with the old Block Group support. The issue that caused was anyone who put - characters in a comment after the Show/Hide line got their comments interfered with by Filtration, this was handled with the # EnableBlockGroups requirement.

The proposal to store all theme data in a single comment immediately following the last line of the block would have the following benefits:

• Doesn't treat any comment on its own line in the middle of a block as a theme tag
• Less likely to cause parsing errors in the future
• Easily identifiable as theme-related data

I think we need a design discussion before implementing a solution to this.

[azakhi]

Agreed, it should be discussed. My opinion is that we should still support theme names in trailing comments because I believe it is the most common (?) way nowadays. And if we continue supporting it, shouldn't we use it so it is one less thing to maintain?
On the other hand having some theme names over, some in trailing comment is also a problem.

[ben-wallis]

I'm kind of interested to know how many people even use themes in Filtration - the original intention of them was for people to distribute .filtertheme files with their filter, but if you google "filtertheme" there's only 2 results. I think maybe it'd be worth doing the minimum work to fix it at the moment (disable theme support for blocks that don't support trailing comments) until we know whether it'd actually be better just to remove themes entirely.

In retrospect I think Block Groups and Themes were actually a mistake to even add to Filtration since they're very bespoke concepts and with filters not naturally having anywhere to store proper metadata other than comments, they're prone to causing parsing issues/bugs/confusion for users. I'd rather Filtration be extremely good at one thing (making item filters easy to edit), than have a load of half baked features that nobody uses tacked on top.

[azakhi]

I can't comment on how many peope use themes but I am using filtration for more than 2 years and themes are one of the things that I find quite useful if not the best feature. I have only edited and used master theme though. Block groups on the other hand are something I have never touched. I think the problem with them is that they bring more problems while trying to solve another. I need to maintain block groups (which can't be done through the program itself), I need to remember which blocks have what groups and I need to come up with a good grouping plan so I can actually benefit from them. If they were disabling blocks instead of turning them to hide and had a way to be managed in Filtration, I would probably use them.
It boils down to the question, is Filtration a tool for filter creators or for people who make couple small changes to existing filters? I wanna quote a feedback here I got from a filter creator, (not exact words) "If I wanted to make a couple of small changes, using Notepad++ would be easier, faster and it would give me more control/freedom". I think it is important for filtration to have features that make changes involving many blocks easy and fast, like themes or sections.
Those are my opinions from the point of view of a person who creates and uses his own filters.

[ben-wallis]

That's a valid point - I kinda forgot that the master theme concept existed (it's been a while since I wrote the theme stuff). I'll have to have a think about possible solutions since I really don't like the split line comments.

[ben-wallis]

Setting a theme has been disabled in the UI in 1.0.0-beta2 as a temporary fix for the following block item types:
MinimapIcon
PlayEffect
SetFontSize

[azakhi]

As said by a Path of Exile developer here:

Another change is that comments (marked with "#") should now work better. Previously they could cause errors in certain places because they were incorrectly interpreted as search strings/values etc. Now, they should work everywhere. If you find somewhere that comments still don't work, let me know.

This issue will probably be resolved in near future.