search

wrye-bash / wrye-bash

A swiss army knife for modding Bethesda games.
https://wrye-bash.github.io
GNU General Public License v3.0
455 stars 79 forks source link

Document and improve behavior of the Filter tag #654

Closed Infernio closed 1 year ago

Infernio commented 1 year ago

Seeing as we have #161 already...

The Filter tag doesn't do what most people seem to think it does. The only thing it does is change what records get merged into the BP when merging a plugin into the BP. So in particular, if you tag a plugin with both Filter and NoMerge, the result is that it won't get merged, and so the Filter tag does nothing. An example of a plugin affected by this is the very popular All Natural - Indoor Weather Filter For Mods.esp. It has the tags {{BASH:C.Climate,Deactivate,Filter,NoMerge}}. The Filter tag here is useless - the way this plugin actually works in Wrye Bash is via the C.Climate tag, because Import Cells will still import data from it in its initData phase.

However, here's where things get really fun. The only reason that works is due to this line: https://github.com/wrye-bash/wrye-bash/blob/c79c03a3955c4f91aa79c0c50b3eafefc70169a0/Mopy/bash/patcher/patchers/preservers.py#L476

Which also assumes that in order for this to work, the plugin must be a Filter!

Clearly everyone here has become confused as to what Filter actually does and what it should do. I can find at least two usages in the LOOT masterlist where LOOT adds both a NoMerge and a Filter tag, but also a couple where it adds a Filter tag and removes a NoMerge tag. WB itself lets the plugin do things even without a Filter tag, while the readme only claims it must be "mergeable" - it never claims it must be "merged in":

https://github.com/wrye-bash/wrye-bash/blob/c79c03a3955c4f91aa79c0c50b3eafefc70169a0/Mopy/Docs/Wrye%20Bash%20Advanced%20Readme.html#L6554

I see two options:

  1. Change the documentation to mention what Filter de facto does and add warnings for plugins that use both Filter and NoMerge.
  2. Change the behavior of WB so that Filter is required for importing data from plugins with missing masters, as everyone seems to assume it is.

I'm leaning towards option 2 to be honest. It's super weird that a non-Filter plugin with missing masters can import data into the BP, and seeing the comment above, the code seems to agree it should be required.

Utumno commented 1 year ago

Thanks for looking into this. I admit I don't quite get merge_records (doFilter and iiSkipMerge in particular). Please if you edit record grups do it on top of ut-647 branch - I am smoothing out some last bugs but then it's nightly time - it really blocks #653 and this one - and #480/#312 of course - I also cherry picked the deleted records commit there (cause I need the should_skip for complex records also) - force pushed (still debugging but close)

Infernio commented 1 year ago

I'm not touching record_groups for that reason exactly :) There is some stuff in record_groups that does need editing for this (really, updateRecords should do filtering as well, which means we should be able to save a ton of code by unifying updateRecords and merge_records), but that can be a pt2 commit once #647 is merged (or at least on nightly)

Utumno commented 1 year ago

unifying updateRecords and merge_records

<3 - no really, this API needs thinning ASAP (see the _process_rec mega hack - or rather don't just fixed another bug in there)

Another one that must absolutely go is that set_changed - more on this soon

Infernio commented 1 year ago

Thinking more on it, it doesn't really make sense to expand filtering to updateRecords. Unifying the two should still be possible and is a good goal though.

Infernio commented 1 year ago

Not quite done yet as it turns out, will be closed with a new commit on nightly.