Mutagen-Modding / Mutagen.Bethesda.Analyzers

A project to diagnose and analyze the health of a mod or a load order
GNU General Public License v3.0
18 stars 5 forks source link

NestedRecordAnalyzer #153

Open Noggog opened 1 month ago

Noggog commented 1 month ago

An example of this is InvalidSayOnceAnalyzer, which wants to resolve its parent and analyze the record in relation to it

This would make a specialized analyzer so the dev doesn't need to resolve the parent themselves, and the refresh mechanisms can be streamlined

Elscrux commented 1 month ago

Would this need to be a special analyzer type, or just a special way of determining the context that is relevant to the topic for re-triggering the analyzer? It might also be possible that there is relevant context from multiple sources as well think.

Noggog commented 1 month ago

This is the reverse of #155, solving similar challenges. It would be a contextual style under the hood, but with extra features. The way I think of these is that we can probably do most of them via ContextualAnalyzer as a fallback. These would be convenience alternatives that have more of the guts handled, so the author can focus more on the goal.

If someone wants to analyze Placed Objects within a worldspace.. then they could use a contextual analyzer, ofc. (everyone can always use a contextual analyzer, as it can do anything). In a similar vein, we still have isolated analyzer styles, even though contextual could also do them. They're just specialized tools on the toolbelt.

But using a NestedRecordAnalyzer would:

It might not work for a crazy analyzer that does want to touch a lot more, and so those might fall back to contextual analyzer again.. being much more complex and error prone, but able to do more.

Elscrux commented 1 month ago

I have similar feelings as in #155 where I think we might not want to have a very specific variant like this. I think we have to see if there are enough benefits that justify the extra engine complexity and amount of options analyzer devs have to consider when creating an analyzer. Resolving additional fields seems convenient although it's also something we can do in analyzers without a lot of boilerplate code. In terms of filtering out specific records that are interesting for an analyzer I think we can't really handle that inside a base analyzer, looking at this example. To only analyze cells inside a worldspace would mean that you iterate all cells and check if they have the IsInterior flag set, or iterating through worldspaces and and grabbing all cells underneath. Or if we're talking about resolving the parent I guess you could also resolve the root parent record of the cell and check if it's a worldspace. In all cases this seems like a quite specific filtering operations where I'm not sure if that could be streamlined in the engine. The resolving parent part and checking if it's a worldspace maybe. But how does it work with InvalidSayOnceAnalyzer which doesn't resolve a parented record but a record that is linked in the topic? In terms of fields of interest we should also be able to handle this via link cache (#152) I think? Also for this one I hope I understood your points correctly, let me know if I didn't!

Noggog commented 1 month ago

Gonna respond to the few related issues here in one place, as we're sort of having a theoretical discussion about them all.

Though what happens if we're starting off in some other record like IFactionGetter but then we're resolving a cell and we're accessing the placed objects inside that? I don't think this specific ParentAnalyzer design would help with that

But how does it work with InvalidSayOnceAnalyzer which doesn't resolve a parented record but a record that is linked in the topic?

I don't think they need to handle these situations. There will always be complex analyzers that need to fall back to ContextualAnalyzer and do everything themselves.

It'd be like having a 3d printer that can print any workbench tool (ContextualAnalyzer). But ofc, it's slow, error prone, makes weak breakable tools, but can technically print any tool you need.

We still might go out and buy a flathead screwdriver for our workbench. It'll be stronger than one we 3d printed, and we'll use it decently often. But what if it's a phillips screw we're working with? Sure, the flathead won't do that. Let's maybe also get a phillips head, too.


Back to our engine here, in a similar fashion... we could get away with not having ANY analyzer styles but ContextualAnalyzer (it's the 3d printer): That's really the only one we need. All the others are just specialized variants meant to handle specific common situations.

For example IsolatedRecordAnalyzer is really just a specialized variant for the common scenario of analyzing a single record in isolation. It's not actually needed and ofc cannot handle everything.

If your analyzer just needs to look at an isolated nested record, relative to its parent.. NestedRecordAnalyzer would be a good choice. (InvalidSayOnceAnalyzer seems to be one of these)

If you're analyzing a cell and want to look at its placed contents, maybe ParentAnalyzer is a good choice, as it'll compile all the Placed things for you in the LO, rather than you needing to iterate and accumulate them yourself. Less code to get wrong or misoptimize.

But you're always going to be able to come up with an idea that these tools wont handle. I don't think that's a reason to not make them as alternatives to choose. Sure, there will be some that need to fall back to ContextualAnalyzer, but that's fine. I still think there's a place for specialized variants to exist. IsolatedRecordAnalyzer is just the most obvious and common one

Elscrux commented 1 month ago

I think it's totally valid to have multiple types of analyzers. I'd like to keep the number down to make sure especially new people don't need to put a lot of thought into what analyzer they should take when there are 10 different kinds of analyzers. IsolatedRecordAnalyzer makes a lot of sense as you often just want to look at an individual record. You don't want to manually enumerate over the records in a mod and it also adds a lot of additional things the engine can do to handle this case well, it makes a lot of sense. NestedRecordAnalyzer could definitely also make sense, I'm just not quite seeing that yet. When we can have some generic analyzer in which we look at any nested records that could definitely be interesting thinking about it. InvalidSayOnceAnalyzer is looking at IDialogResponsesGetter nested under IDialogTopicGetter (so we need to get the context here which would then be handled by the engine). This is used to resolve the link to the IQuestGetter the dialog is part of, although it's important to note that quests are not part of the nested structure but separate so that's not important. So examples of nested structures could be:

and all of these could be handled with one generic analyzer (+ maybe a second variant with the same name and one more generic in the middle).

Noggog commented 1 month ago

I think it's totally valid to have multiple types of analyzers. I'd like to keep the number down to make sure especially new people don't need to put a lot of thought into what analyzer they should take when there are 10 different kinds of analyzers.

Definitely a good point. Could definitely get overwhelming trying to find an analyzer type... and there's 50 choices, haha. One angle to consider, though, is that having many analyzer types can also be a boon to newcomers.

If someone new comes along with "an idea do check X for Y", we can say "hey, make an IsolatedRecordAnalyzer<X>, implement the functions it wants, and you'll be good to go!" The fact that it exists helps find a nice home for the user where the complexities are hidden away leaving them just with the record they wanted to check for Y. It's hard for them to get anything wrong, and they only need to write several lines beyond the IDE generated bootstrap of the interface.

If we recommend the ContextualAnalyzer to everyone for everything, then we also have to give more tutorials and guidance on all the extra bits they have to do to "morph" it into the desired style of patcher. For a ParentRecordAnalyzer.. stuff like how'd they'd accumulate placed objects correctly from a cell over many overriding mods. The proper way to register the things that they queried for. maybe a few other complexities. Not a showstopper, but comes with a tutorial cost.

if we can recommend the ParentRecordAnalyzer to that same person, they get fed the cell + all the placed objects handed to them in the parameters, and perhaps the registration is handled/streamlined for them or demanded for loudly... that's a lot less to learn to get started on their idea.

One route we could consider is in the docs covering the heavy hitters up top highlighted: IsolatedRecord, ContextualAnalyzer, etc. And then in a subsection side area, we could list the more obscure specialized alternatives for consideration. Since they don't NEED to find the perfect match, they can just go to Contextual if they don't care enough to find the perfect shoe fit.


I do think we can wait on doing these specialized analyzers until later. No need to do them this early while we're still figuring things out. We might have some more color and knowledge of whether they're needed later on

Elscrux commented 1 month ago

I think it's a good idea to have some highlighted analyzers and some more specialized that are not used as much when they are relevant.

Similar to what I wrote above, generic nested analyzers could be good, especially if they also have the option to resolve the final form of the nested structure based on the load order as it is after all additions/deletions/modifications.