SteveTownsend / ImmersiveEquipmentMeshGen

Skyrim SE Synthesis Patcher for Immersive Equipment Display - mesh generation
GNU General Public License v3.0
12 stars 12 forks source link

CTD upon entering windstad manor #3

Closed parsafaramarzi closed 2 years ago

parsafaramarzi commented 2 years ago

with generated meshes enabled in my mo2 left panel i crash in windstad manor which is my main house and i have a lot of weapons on display cases and mannequins and weapon racks (which i believe is causing the crash). if you need further info regarding this let me know.

SlavicPotato commented 2 years ago

One or more meshes probably got broken during patching, it's likely there was something wrong with the originals. Can't tell you more until you find the culprits.

parsafaramarzi commented 2 years ago

hi found the culprit its the dawnbreaker.nif and if im correct about how the patcher works its using the meshes from this mod

parsafaramarzi commented 2 years ago

the first person model is broken too i just tried equipping it.

SteveTownsend commented 2 years ago

I am not a mesh expert but it seems likely to me that the animations of this mod might need mapping to work with item positioning mods.

SteveTownsend commented 2 years ago

From that mod's page:

anything that changes the mesh of the weapon is incompatible.

So you are out of compatibility for that, sorry. You'd have to blacklist this mesh in the patcher.

SlavicPotato commented 2 years ago

The issue seems to be niflysharp breaking animation data (sse mesh optimizer which uses nifly doesn't, it's why I assume it's related to the C# wrapper).

The breakage is far worse when 'optimize' is set (NifSaveOptions) but even without it the animations will come out broken, even though, AFAIK, the patcher never touches that data.

edit: grammar

SteveTownsend commented 2 years ago

The AllGUD patcher had a check to special-case (ignore) "Dynamic" (vs "Static") models, I wonder if that is required here to avoid messing this and others up?

SlavicPotato commented 2 years ago

The patcher could just ignore nifs with NiControllerManager instances, that said - it shouldn't have to. Seemingly the issue is with the nifly library/C# bindings. It breaks the data if doing nothing more than open/save. I haven't tested the nifly lib itself, but nif optimizer and nifskope both work fine.

SteveTownsend commented 2 years ago

Ignoring them is the quick fix. I never investigated Dynamic Display for the AllGUD patcher so this would be new work. At least I have a good test case. I don;t see getting to this in March though.

On Tue, Mar 1, 2022 at 5:47 PM SlavicPotato @.***> wrote:

The patcher could just ignore nifs with NiControllerManager instances, that said - it shouldn't have to. Seemingly the issue is with the nifly library/C# bindings. It breaks the data if doing nothing more than open/save. I haven't tested the nifly lib itself, but nif optimizer and nifskope both work fine.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/issues/3#issuecomment-1055936150, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNC7NGP23QVNL3LMRIA3U52NANANCNFSM5PBDUQ7A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.*** com>

parsafaramarzi commented 2 years ago

thanks for the help!

parsafaramarzi commented 2 years ago

before closing this issue could i ask how i should blacklist these two meshes? i went about putting "meshes\weapons\dawnbreaker\dawnbreaker.nif,meshes\weapons\dawnbreaker\1stpersondawnbreaker.nif" in but the meshes were generated anyways

SlavicPotato commented 2 years ago

I'm not sure what exactly allgud refers to as 'dynamic display' but I am sure that it's not at all relevant here - the mesh needs to be left intact, no other changes should happen except for adding the additional scabbard node. Optimization and sorting should be turned off or atleast made optional. nifly(sharp) absolutely needs to be able to save the mesh as it was read without having to implement hacky workarounds, otherwise it's of little use in this case.

SteveTownsend commented 2 years ago

I can disable the optimize-on-save easily enough. Do you believe this will fix certain currently-broken output meshes?

AllGUD Dynamic Display refers to any mesh that has a controller in it.

SteveTownsend commented 2 years ago

before closing this issue could i ask how i should blacklist these two meshes? i went about putting "meshes\weapons\dawnbreaker\dawnbreaker.nif,meshes\weapons\dawnbreaker\1stpersondawnbreaker.nif" in but the meshes were generated anyways

If you want to use complete relative paths you should use either / or \\ as the separator. The patcher uses wildcard matching so just dawnbreaker as a filter ought to work

SlavicPotato commented 2 years ago

I can disable the optimize-on-save easily enough. Do you believe this will fix certain currently-broken output meshes?

It won't, i've already tested, though there's far less fallout.

I've tested nifly (current main branch), saves/loads work fine. niflysharp v1.2.3 also works as expected, the issue is present in 1.2.8 and newer.

One symptom is that the string table is reordered but indexes are not updated (for example on NiTextKeyExtraData, NiControllerSequence)

I've kept the test as simple as possible:

using nifly;

NifFile nif = new();

if (nif.Load("X:\\tmp\\dawnbreaker.nif") != 0)
{
    throw new Exception("load failed");
}

NifSaveOptions so = new();

so.optimize = false;
so.sortBlocks = false;

if (nif.Save("X:\\tmp\\dawnbreaker_o.nif", so) != 0)
{
    throw new Exception("save failed");
}

Console.WriteLine("ok");
SlavicPotato commented 2 years ago

https://github.com/SteveTownsend/nifly/commit/de4ff4b33861388908b5477bf9ba378ba29b8bce seems to be the last functional commit.

SteveTownsend commented 2 years ago

OK thanks. Maybe I just need to sync up to current nifly again. I'll have a look at what changed and sync to ousnius code, to start with.

On Sun, Mar 6, 2022 at 2:55 AM SlavicPotato @.***> wrote:

@.*** https://github.com/SteveTownsend/nifly/commit/de4ff4b33861388908b5477bf9ba378ba29b8bce seems to be the last functional commit.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/issues/3#issuecomment-1059914009, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNC5ZYM274QL7K734BZDU6RQFPANCNFSM5PBDUQ7A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.*** com>

SteveTownsend commented 2 years ago

SteveTownsend/nifly@de4ff4b seems to be the last functional commit.

I've reviewed my nifly fork and niflyswig main. There really has been zero risky change in niflyswig in the period of interest. On the other hand, ousnius/nifly has changed quite a bit since I last synced to it at the end of 2021. CHanges include refactoring and a bug fix for block sorting, which could well affect the optimize-on-save path in niflysharp. I'll PR ousnius/nifly:main into my fork and see where we stand.

SlavicPotato commented 2 years ago

I've tracked down the issue, see PR.

SteveTownsend commented 2 years ago

That's great, thanks. I have a PR in flight to sync my main to ousnius main, and then I'll incorporate this and make a build of the niflysharp package and the two patchers that use it.

I would definitely have struggled to resolve this so quickly - my main value-add here was in code porting rather than mesh internals.

On Tue, Mar 8, 2022 at 10:32 AM SlavicPotato @.***> wrote:

I've tracked down the issue, see PR.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/issues/3#issuecomment-1061905197, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNCZ3D2LNTSRNQNGUL2TU65XJZANCNFSM5PBDUQ7A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.*** com>

SlavicPotato commented 2 years ago

I compared the two projects and reviewed the changes which seemed arbitrary and related to string refs, stumbled onto that almost immediately. Consider removing those changes (is there some swig related reason for why it was added?). I've marked a few stuff in BasicTypes.hpp for review.

edit: typos

SlavicPotato commented 2 years ago

I see some other edits which test if stuff is empty before doing something which could result in undefined behavior unless you're absolutely sure what you're doing.

SteveTownsend commented 2 years ago

yes those are not in the ousnius version. I am not sure how I got out of sync. but merge to latest and then careful manual reconciliation is needed.

On Tue, Mar 8, 2022 at 11:05 AM SlavicPotato @.***> wrote:

I see some other edits which test if stuff is empty before doing something which could result in undefined behavior unless you're absolutely sure what you're doing.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/issues/3#issuecomment-1061940540, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNC5RVFEVP4AGJTDCWN3U653ELANCNFSM5PBDUQ7A . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you modified the open/close state.Message ID: @.*** com>

SteveTownsend commented 2 years ago

this should be fixed in v1.3.0 of the patcher, regen meshes and retry