Synthesis-Collective / SynthEBD

Port of zEBD to Mutagen
31 stars 7 forks source link

Suggestions for MixIn assignments #85

Closed ghost closed 1 year ago

ghost commented 1 year ago

Hey Piranha, I'll leave these suggestions as a separate issue for better organization, a reminder, and because I think they would offer great flexibility to modding in a next release:

These would be useful to distribute textures in order to 'fill-in' missing textures from other asset packs. For example, HeadDetail textures or those 3BA etc textures which some skin mods do provide, but others don't. This way it would be possible to create a MixIn pack of all those 3BA textures from the 3BA mod, and assign them as a complete fill-in or complement to compatible packs that lack all those textures, or who just provide the diffuse. This should be useful for providing missing HeadDetail textures to other asset packs too.

Thanks!

Piranha91 commented 1 year ago

Added priority assignment in 0.9.1.

For the global probability assignment, do you mean something like for Headpart categories that controls a yes/no "will this mix-in get assigned, even if the target NPC fully complies with its distribution rules"?

ghost commented 1 year ago

Something like that, yes. HeadParts have this option for distribution probability:

image

In my mind, for MixIn, the behaviour would be something like this:

The first one would be useful to distribute 3ba _etc textures for those asset packs that doesn't provide them, by letting MixIn have a lower priority (filling in first), and then letting Primary Asset Pack provide the rest (even superceding any conflicting textures, since Primary may provide textures better suited for).

The second one would be useful to distribute those mods which only provide standalone normal textures. This way one could have the option to assign them to all Primary Asset Packs without having to customise and add them to each Primary Asset config rules.

Piranha91 commented 1 year ago

Ok. The priority is already able to be set in 0.9.1. - if a mix-in comes first and a primary comes second and they both hit the same destination path, the primary will win. If a mix-in comes after primary in the assignment order, the mix-in will win. I can add in a global distribution probability for mix-ins similar to headparts.

Piranha91 commented 1 year ago

Alright, in version 0.9.1.2, the probability in the whole-config distribution rules for a Mix-In is now a percentage that dictates the probability that the given Mix-In will get assigned. Let me know how it works for you.

ghost commented 1 year ago

Hey, testing it know with Primary Asset packs followed by one Mix-In with only custom normal maps.

There's just something which I didn't quite understand yet. During Mix-In assignment, some normal maps are excluded with this message:

"is invalid because its allowed descriptors do not include any of those annotated in the descriptors of assigned preset"

So, when deciding the Mix In assignment, the patcher compares the custom normal maps body descriptors with those that resulted from the Primary Asset pack rules, or from the previously selected Bodyslide?

Piranha91 commented 1 year ago

Should be the bodyslide. In the asset ordering menu, "primary" and "body shape" are linked and assigned at the same time (this is so that the patcher can execute its "meta loop", for lack of a better term - it assigns a valid combo, tries to assign a body shape, and if no shape can be assigned, backtracks and tries another valid combo, until all combinations + body shapes are exhausted. This could get really expensive, and obnoxious to code, if it had to backtrack through mix-ins as well, so it only does the "meta loop" for primary packs).

For mix-ins, the way it should work is as follows:

At least that's how it's supposed to work - I've been a bit bandwidth-limited recently so if that's not consistent with what you see, let me know.

ghost commented 1 year ago

Hummm thanks for explaining... however, just to clarify, and to check if the logic seems ok... I think I'm not following SynthEBD's logic for bodyslides...

I have the following Body Normal, with these descriptors:

image

What is the resulting boolean logic here which SynthEBD considers?

I built the rules with the first logic in mind, but I suspect SynthEBD applies the second one. I'm trying to understand that better because, running the patcher with those rules from the screenshot, the patcher chose for that body normal, a bodyslide with these descriptors:

image

Which shouldn't have happened, since the body normal is not intended for a 'fit' body.

Later on, when processing the following Mix-In with custom normal maps, the patcher assigned one with these descriptors:

image

Which implies it followed the logic from the Primary Asset pack (which IMO should be the intended behaviour) and not from the Bodyslide (as you explained)... šŸ¤”

Piranha91 commented 1 year ago

To clarify, the first screenshot is the bodyslide descriptors of the primary asset pack? If so, something is going wrong with the bodyslide assignment - it's the first logic which should be implemented (match build AND match chest). Note that this can "break" if the bodyslide is missing a descriptor entirely - if it has "build: fit" but no chest annotation at all, then the patcher won't try to match the chest annotation and will just accept the bodyslide as valid, so it's on the user to make sure bodyslides are fully annotated. Can you upload the verbose log for that NPC?

The mix-in logic is "correct" in the sense that, if the patcher is working properly, the assigned bodyslide complies with the Primary descriptors, which means that if the mix-in complies with the bodyslide, then it also complies with the primary.

By the way, do you think it would be useful to add a button to import/export bodyslide annotations so that users can share theirs without having to hand-annotate each?

ghost commented 1 year ago

Here's the log:

Bandit Cryomancer (EncBanditIce06HighElfF) 0010D7-OBIS SE.esp.xml.zip

Just disregard that report of the mix-in running after. It seems to have behaved correctly (comparing to the bodyslide), and did not assign a wrong normal. I got confused looking at two logs at once šŸ˜ž Though I think it would be better if the patcher compared to the resulting rules of the primary asset packs, instead of bodyslide...

And yeah the first pic is from the normal map of the primary asset pack.

A way to export bodyslide annotations would be useful indeed, maybe something similar to what is already done with race groupings and attribute groups.

On another note, I think I'm also running into patcher or/and logic issues when creating an attribute group (for example, female mature) and adding multiple groups (e.g. female age 40, female age 50)... I'll investigate a bit more and report on another issue then...

ghost commented 1 year ago

Here are the annotations used for primary asset pack:

image

And for the bodyslide selected by the patcher:

image

Piranha91 commented 1 year ago

Hmm... let me improve the verbose logging a bit and make a new release in just a bit - it won't fix the issue but might make it a bit easier to diagnose for the next one. Btw, I noticed that for assets there are log entries like:

Subgroup BN.CBBE3BA.Musc (Muscles)  is invalid because the NPC matches one of its disallowed attributes: 
{Group: [Cannot be Muscular]}

But for bodyslides there are ones that are like:

Preset Warrior Princess 3BA is invalid because the rules for its descriptor Age: Younger is invalid because the NPC matches one of its disallowed attributes: 
{Group: []}

Do you have an unnamed attribute group in your general settings, or is this a bug?

ghost commented 1 year ago

First one is fine, it's a list of npcs which shouldn't get muscular body normals. The second one however, looks like a left over bug.

Checking now, there is a disallowed npc attribute rule there, which used to have an attribute group (inherited from general settings) which I have recently renamed or deleted, can't remember which... it looks like after an app restart, the group was gone from the rule, but the rule remained there, empty..... weird...

Piranha91 commented 1 year ago

Whoops. I was baffled how this was happening because I spent hours upon hours validating the algorithm - turned out I broke it in 0.9.1 when introducing the Asset Ordering feature - the descriptor rules going to the bodyslide selector were specifically missing those from the Primary asset pack, and only passing the mix-ins. Thank you for catching this. Fixed in 0.9.2. Also added a validator to catch blank attribute groups (and other attribute types), since if an Allowed Attribute is blank the NPC will never be matched.

ghost commented 1 year ago

That new validator is quite useful, it caught a lot of blank attr groups across many asset packs now, thanks!

The new verbose logging is great too, very detailed, thanks. However, I fear the problem is not solved yet... here are some log samples for multiple NPCs (no mix-ins involved on these):

NPC1 (no mix-in): Applied descriptor rules: BFN.4K.U.S.D: Allowed Descriptors: Build: [Slim, Curvy] | Chest: [Small, Medium, Big] ... Chose BodySlide Preset: Athletica
Contained descriptors: Build: [Fit, Muscular] Chest: [Small]

NPC2 (no mix-in): Applied descriptor rules: BN.M.D: Allowed Descriptors: Build: [Average, Slim, Curvy, Chubby] | Chest: [Big, Huge] ... Chose BodySlide Preset: Moderate Curvy Contained descriptors: Build: [Average, Slim] Chest: [Flat]

NPC3 (no mix-in): Applied descriptor rules: BN.Crv.Smo: Allowed Descriptors: Build: [Average, Slim, Curvy] | Chest: [Big, Huge] ... Chose BodySlide Preset: CBBE-3BBB The Sensual Lady_By Bacetrica Contained descriptors: Build: [Average, Curvy] Chest: [Medium]

NPC4 (mix-in seems to have matched primary asset descriptor rules, but primary and bodyslide didn't): Applied descriptor rules: BN.D.NH: Allowed Descriptors: Build: [Slim, Curvy, Fit] | Chest: [Small, Medium, Big] ... Chose BodySlide Preset: 3BA One Body Contained descriptors: Build: [Average] Chest: [Big] ... Successfully generated combination: CBBE 3BA Custom Normal Maps:HN.CBBE3BA.Def|BN.CBBE3BA.NipTh|HaN.CBBE3BA.Def Applied descriptor rules: BN.CBBE3BA.NipTh: Allowed Descriptors: Build: [Average, Slim, Curvy] | Chest: [Medium, Big, Huge]

NPC5 (no mix-in): Applied descriptor rules: BN.M.S: Allowed Descriptors: Build: [Average, Slim, Curvy, Chubby] | Chest: [Small, Medium] ... Chose BodySlide Preset: Full Apex Girl Growth Contained descriptors: Build: [Average, Fit, Muscular] Chest: [Big]

NPC6 (this one seems ok, matching all three descriptors from primary, mix-in, and bodyslide): Applied descriptor rules: BN.M.D: Allowed Descriptors: Build: [Average, Slim, Curvy, Chubby] | Chest: [Big, Huge] ... Chose BodySlide Preset: Creamy Curves
Contained descriptors: Build: [Curvy, Fit, Muscular] Chest: [Big] ... Applied descriptor rules: BN.3BA.PNBBSB: Allowed Descriptors: Build: [Average, Slim, Curvy, Chubby] | Chest: [Medium, Big, Huge]

I wonder how convoluted the code may be getting now... making bugs hard to spot :(

Regarding all these possible combinations of primary asset and mix-ins, don't you think a simpler and better strategy would be to let the bodyslide selector run only at the end of primary asset and mix-in(s) combinations, letting the last body descriptor rules win, following the user defined Distribution Order? I mean, something like:

For example:

1st MixIn: Build: [Average, Curvy, Chubby] | Chest: [Medium, Big] 2nd Primary Asset: Build: [Average, Chubby] | Chest: [Big] | Race: [Nord]

3rd MixIn: Build: [Average] | Chest: [Medium, Big] | Final combination: Build: [Average] | Chest: [Big] | Race: [Nord] Finally, run bodyslide selector based on final combination

Another example:

1st MixIn: Build: [Average, Curvy, Chubby] | Chest: [Medium, Big] 2nd Primary Asset: Build: [Muscular, Fit | Chest: [Big, Huge] | Race: [Nord]

3rd MixIn: Build: [Average] | Chest: [Medium, Big] | Race: [Breton]

What do you think? Does that makes sense?

Piranha91 commented 1 year ago

I'd be reluctant to make that change for a couple reasons

1) The "main product" of SynthEBD is the primary asset pack. The way the algorithm works now gives priority to to making sure the primary asset pack works with the assigned body shape by backtracking and selecting a new primary if the current one can't be made to work with any body shapes. If I separate them out, it adds additional complexity. What happens if no valid body shape can be assigned at the end? Does the patcher backtrack through everything and try to find an alternate combination at every point? What happens if, for a given mix-in, the descriptor rules are completely incompatible with the selected bodyslide? Should the mix-in be removed (e.g. "declined"), or should the patcher throw its hands up and choose a bodyslide irrespective of descriptors? The way it is now gives a lot of flexiblity - if the user wants to prioritize primary, they put it first and then all mix-ins have to comply with those descriptors. If mix-ins have descriptors that need to be prioritized, they go in the assignment order before primary/body shape. I think this is the most customizable way to do it, and...

2) It works in my testing. I made verbose logs for about 10-15 NPCs, and each of them are getting bodyslide assignments that are consistent with their primary asset descriptors. In your cases above, are you sure they were generated with the latest SynthEBD? If so, could you upload some of the full logs so I can take a look at what's going on?

3) If there's a burning need to change it I might come back and do it, but I've been burning out on SynthEBD for a while and at this point I really want to just get a working version published to the nexus, take a break, and remember some of my other hobbies. I've mentioned this before on Discord so apologies if I sound like a broken record but for the last 4 months I've had two jobs, so it feels like I go from job #1 to job #2 to coding SynthEBD in the evening with no rest, and it's kind of burning me out. At the same time I know that at least one person is coding an at-runtime texture patcher (probably less customizable than SynthEBD but perhaps easier to install), and I'm already "competing" (if that's the right word) with RBT, so I want to get this out before it completely loses relevance. That's the main reason I'm reluctant to change the core algorithm - I'd rather just do whatever needs to be done to patch the current one and make it work as well as possible, and release.

ghost commented 1 year ago

Yeah, nobody wants to feel burnt out, I can totally relate to that... even less so on a hobby which should actualy improve our mood instead of drain it... Working two jobs is also a pretty taxing thing :( Really hoping here that things work out for you IRL :)

The way SynthEBD is today already gives us a tremendous amount of flexibility, I don't think those other mods could even compare to SynthEBD šŸ‘

So, yeah let's try to iron out current bugs first and get this release out asap so you can take a break. I promise I'll stop being such a heavy feature requester šŸ¤£ You've already accomodated a lot of my requests, even those which are just quality of life and quite superfluous, and for that I'm grateful :)

Unfortunately, I've already deleted my log folder since it was growing bigger and bigger, but I did manage to luckily save a full log for one NPC here:

Ahlam Ahlam 013BBE_Skyrim.esm.xml.zip

The Bodyslide entries are these:

image

image

And yeah, I just double checked my MO2 executable and it is calling SynthEBD 0.9.2 (humm, waybe a way to check the app version from the main menu could help).

I'll leave the patcher running again tonight for some new logs, and also do some more investigating on my side.

Take care!

ghost commented 1 year ago

By the way... now that I've posted those bodyslide pics... I've realised that maybe the patcher is correctly assigning the bodyslide, but actually printing the descriptors of the OTHER bodyslide which have the same name? šŸ¤”

Piranha91 commented 1 year ago

That actually seems quite possible. I wrote some code this evening to try to deconvolve that. I'm almost done with the BodySlide Annotation Exchange - just need to write the UI and test it a bit. Once that's done I'll make a new release, and we'll see if your hypothesis is correct :)

Piranha91 commented 1 year ago

0.9.3 should be able to distinguish between BodySlide duplicates. Mind taking a look at the patch log and seeing if the issue persists?

ghost commented 1 year ago

Humm, now I get a bunch of errors telling me to rename all my duplicated bodyslides... this will take a while...

Piranha91 commented 1 year ago

Yeah unfortunately that's the only way to track them individually. If it helps, I could write a quick function to auto-rename duplicates. Actually let me do that really quick.

ghost commented 1 year ago

Thanks Piranha that would come in handy.. I've got dozens of bodyslides here. I did try to come up with a quick script here at work, but json parse eludes me...

Piranha91 commented 1 year ago

0.9.3.1 is out, automatically renamed duplicates (but does not address your specific NPC assignments issue).

ghost commented 1 year ago

Well, good news, all my bodyslide dupes are numbered now, thanks a lot :)

Bad news, bodyslides are still being wrongly assigned šŸ˜¢

Here's an example: Imperial Beneficiarii 1ImperialBeneficiariifFEMALEVAR1Argonian 014E04_Rank and File.esp.xml.zip

Applied descriptor rules: 
BN.B: Allowed Descriptors: Build: [Average, Slim, Curvy, Chubby] | Chest: [Medium, Big, Huge]
...
Chose BodySlide Preset: North BattleMaiden 3BA

Contained descriptors: 
Build: [Fit]
Chest: [Medium]

My completely uneducated guess is that patcher is somehow matching Build XXX OR Chest YYY instead of Build XXX AND Chest YYY šŸ¤”

While checking logs, I've also found some issues with specific npc assignments and the new mix-in features. It seems that Forced Primary Asset and Bodyslide settings are kept, but any forced mix in definition is completely ignored. The patcher seems to keep said settings, and then throws the NPC into the common patching pipeline, following the distribution order set on "textures and meshes" tab. We kind of already have a way to input multiple mix ins there, so maybe it's just a matter of having the patcher keep settings and follow that order during assignment?

Oh, and about that weird specifc npc assignment problem I reported before, I did some new textures assignments here and there, and the problem hasn't happened again, yet...

Piranha91 commented 1 year ago

I see the problem; you were pretty close. The issue is with "category forgiveness". The way I originally intended matching to work was, if you have an asset set which assigns Allowed Descriptors:

{ 
   hips: [wide],
   belly: [round]
}

and a candidate bodyslide was only tagged with

{
   hips: wide
}

the patcher was supposed to "forgive" the missing "belly" category and consider the bodyslide matched, with the assumption that if the user cared about matching the belly they would have annotated the bodyslide with it. I made a mistake in the algorithm which resulted in the OR logic that you're seeing, where the second category would be forgiven even if the bodyslide DID have it and it didn't match. I fixed that, but then I figured that might not be the way people want the patcher to run (in fact, I'm not sure it's how I would want the patcher to run now). I think I'd rather be more thorough with the annotations and make the patcher match each category, and reject the bodyslide if it's missing any category.

Tomorrow I'll expose a UI setting that lets the user toggle between which behavior they want for Allowed Descriptors (Disallowed will continue to reject a bodyslide if ANY category has a match). Which behavior do you think should be default for Allowed? I'm thinking "match all categories" rather than the originally intended "match same categories". Thoughts?

ghost commented 1 year ago

Hummm, makes sense now! I've always struggled to understand the patcher logic for bodyslides, since it always seemed to not behave as I wanted...

So, now that the OR bug is fixed, I think your first assumption is reasonable, and given the default Category recommendations SynthEBD ships with (Build/Chest), same categories matching should be kept as default. I think it's useful to keep category forgiveness for those categories which are not explicitly annotated on an asset pack's allowed rules. My assumption here is to not hinder the user who has bodyslides with a lot of categories set, and is trying to annotate a very simple asset pack.

Let's say a user has all bodyslides annotated with BUILD, CHEST and AGE categories for example. He then has an asset pack with textures for build and chest varieties, but not for age (like young/mature normals). It would only be needed to annotate the textures with allowed build and chest categories, and let the patcher accept any age category. Now, for asset packs that also comes with young/mature normals, the user would then add age category on allowed descriptors for those.

Oh, and everything I said is considering category matching only. After this initial matching, categories values should still be processed with OR operators regardless, imho. Since many bodyslide shapes may be eligible for one or more normals, this allows bodyslides with two values (say, fit and slim) to match any texture annotated with fit or slim.

The whole matter here is that the user has all the freedom to choose how to organize categories and values. So, in the end the best solution here imho, would indeed be a toggle to let the user choose how the logic behaves (match all/match same)...

Piranha91 commented 1 year ago

Oh thatā€™s an interesting point. The way I started coding it yesterday has the toggles living in the OBody misc menu and affecting all asset packs, but you think each asset pack should be able to set those toggles for itself?

Edit: I think in the situation you describe, that behavior would happen even without Forgiving. The forgiving will always be aimed Subgroup -> BodySlide, not vice versa. In other words, if the BodySlide has an extra Age category which is not in the Subgroupā€™s Allowed/Disallowed Descriptors list, the patcher will always ignore it - it only cares about the rules set forth by the subgroup. The Forgiveness applies to if the subgroupā€™s allowed/disallowed descriptors have a category that the bodyslide does not. So in your example it would apply to the reverse scenario: if a subgroup has allowed descriptor Age: old, but the bodyslide hasnā€™t been annotated with any Age category, the toggle will decide if the patcher will consider that bodyslide to be a match.

And yes, completely agree on OR logic for values; thatā€™s how itā€™s been working and how it will continue.

ghost commented 1 year ago

Ahhhh I'm sorry, I misunderstood the directions! Now I agree with you! If a subgroup has an allowed category descriptor which the bodyslide lacks, then sure, default behaviour should be to NOT consider that bodyslide. Sorry for the confusion. If need be, the user should toggle the alternate behaviour.

Even though I'm usually prone to more flexibility (which a toggle for each asset pack allows), I fear it may lead to some confusion and code complexity. I think that, for now, it's better to follow the strategy of one global rule behaviour set for bodyslide matching in the obody misc menu. My reason for that is that the user should choose one strategy and stick to it.

Piranha91 commented 1 year ago

I believe this should now be fixed in 0.9.4. Please let me know if it's still not behaving as expected - it seems like you have a more richly annotated set of bodyslides to test with than I do. Apologies for soliciting feedback and then doing something different - I thought about it some more and realized that every time I debate between doing something simple and doing something customizable and choose the simple approach, someone later asks me to implement the ability to do the customizable thing. So, I decided to just implement it from the beginning in this case.

ghost commented 1 year ago

Thanks Piranha, will try to test this weekend. Iā€™m on 1.6.640 with a new save since october I thinkā€¦ havent had any crashes related to bodyslides or autobody/obody. I did made the switch from autobody do obodyng, and from .ini to.json back inā€¦ december I think, also without any issues. Are you on latest ObodyNG? I remember reading somewhere that the author claimed to have done some changes to its json format. Maybe that could be an issue? Iā€™m still on an earlier obodyng version for fear of incompatibilities with synthebd generated json.

Piranha91 commented 1 year ago

Thanks! I actually realized what the problem was and deleted thinking you hadnā€™t read my post yet - as usual it was me being dumb - somehow my RaceMenu AE mod got dragged above my original RaceMenu SE in MO2. No crash once I disabled the SE version in favor of the correct one. Btw I think Iā€™m on the latest version of OBodyNG - Iā€™ll take a closer look tomorrow but I didnā€™t notice any issue with BodySlide distribution. The function SynthEBD calls is the same in both the original OBody and NG; any changes to the json format are handled internally by OBody. SynthEBD just tells it the name of the bodyslide to apply to a given NPC.

ghost commented 1 year ago

Oh, no apologies needed, I was just worried if it wouldn't be too much of a coding hassle for little benefit, but the more flexibility the better :)

Finally had some time to test it today, and I think we can finally close this issue šŸ‘ I've been checking logs for many npcs, and everything seems to be matching really fine now, great job!

However, I've now found some problems related to specific npc assignments, which I'll report on another issue to not clutter this one.

Thanks!