SteveTownsend / ImmersiveEquipmentMeshGen

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

Disabled optimization and sorting, removed unused stuff #7

Closed SlavicPotato closed 2 years ago

SlavicPotato commented 2 years ago

Well, no, since the issue is rooted in nifly (see the other PR). This one is just to disable the optimizations/sorting which should've been done anyway considering were messing with the original meshes and not generating copies.

SteveTownsend commented 2 years ago

OK, I'll sort out nifly as soon as I can then

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

Well, no, since the issue is rooted in nifly (see the other PR). This one is just to disable the optimizations/sorting which should've been done anyway considering were messing with the original meshes and not generating copies.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/pull/7#issuecomment-1061938577, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNC2SEG6C5DUUDKSXN7LU6525NANCNFSM5QGXCZ2Q . 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 commented.Message ID: @.***>

SteveTownsend commented 2 years ago

SteveTownsend/nifly:main tagged as v1.3.0 ought to be closer to ousnius and includes your fix for the bad concept. Now we have to work through this. I don't understand why any of this is needed, really, apart from enabling mesh transformation for the case where the mesh has a controller. I am fine with removing code that is unused, but real/new logic might need to be added to make those meshes work. The example we know about is the Dawnbreaker mesh that was raised as an issue on this Github.

There are comments in the AllGUD script which talk about why e.g. excluding blood mesh blocks is done. I am worried that if we take that out, that will introduce other CTDs - I saw that first hand when testing the AllGUD patcher. See below. Terminology: Dynamic Display <=> mesh has controller <=> bUseTemplates = False

        rbTemplate.Hint :=
            'A Static Display copies only the most necessary blocks into a template file to simply display a sheathed weapon'#13#10
            'This removes things like Intermediary blocks, Blood Splatter blocks, Physics Data, and Animated Parts, which normally aren''t necessary for displays.'#13#10
            'There are of course exceptions, like the exceptional Eyecandy Staff of Magnus or Elianora Master Wizard''s Staff'#13#10
            'Until HDT equipment is updated for AllGUD nodes, you''ll likely want MOST of your weapons to be static, with a few exceptions for Dynamic'#13#10#10
            'The main Benefits of this option are:'#13#10
            'Minimal file size. Which, given the number of meshes that you have to generate, can be quite the difference'#13#10
            'Guaranteed Perfect Mirroring. NiTransformControllers will not produce the expected end-result when using the ApplyTransform method of Mirroring Shapes';
            'A Dynamic Display makes as few edits as necessary to convert to AllGUD format, generally resulting in a larger than necessary file size.'#13#10
            'The most notable blocks which are not removed in a Dynamic Display are:'#13#10
            'NiTransformController Blocks for moving parts, see Eyecandy Staff of Magnus or Elianora Master Wizard''s Staff'#13#10
            'bhkCollisionObject/bhkRigidBody Blocks, which are used for physics including HDT (correct me if I''m wrong)';

The relevant logic in the script is driven by this flag, which interacts with the meshHasController flag

            bUseTemplates := rbTemplate.Checked;
            if bUseTemplates then begin
                slPreferences[indexTemplate] := 'True';
                Log('  will create Static displays from the template');
            end else begin
                slPreferences[indexTemplate] := 'False';
                Log('  will create Dynamic displays by duplicating the source');
            end;

I can dig into this some more as time permits.

I would prefer to release none or very little of this PR at this point, because I think the nifly changes will fix more obvious issues.

SlavicPotato commented 2 years ago

allgud loads custom, preprocessed meshes, none of that is relevant here. IED loads the original weapon mesh as-is, with all the blood/controller and everything else left intact. Animations on displays work. Blood meshes are there (never made visible by the game, but still). We're only adding a clone of the scabbard node - if the original had blood meshes (which I don't see why it would), that's fine, the clone can have them too. If a controller manager on the scb parent targets the orignal scabbard node (just as unlikely), that's fine too - it won't reference the clone so the worst that could happen is that it won't be animated. I'm not sure what happens if the manager is a child of the original scabbard (unlikely?), presumably nifly is able to clone it properly? Any 'loose' controllers (like shader prop controllers) on the scabbard need to be cloned properly by nifly too.

Bottom line: blood meshes should be left alone and the controller code in ApplyTransformToChild doesn't really do anything atm except issue a warning. I've just commented it out. NiTransformController instances need to be properly cloned by nifly.

SlavicPotato commented 2 years ago

I'm guessing there could be issues when applying transforms on scabbard nodes and their children with NiTransformController instances? But again, since the current code doesn't seem to handle or abort in such cases, I suppose it's not relevant in regards to that PR.

SteveTownsend commented 2 years ago

OK. You are definitely more conversant with the internals of the meshes than I am, so I will merge this in tonight and make a patcher release aligned with the updated nifly.

SlavicPotato commented 2 years ago

Judging from the comment @IsBloodMesh, it seems the justification to remove those is to save space (makes sense when piling on who knows how many extra meshes, not /w this patcher though since scabbards won't have them unless someone did something really stupid) and one bad mesh from a mod causing crashes. Since there's no sure way to actually match those, we could actually do more damage with that here in case of a mismatch.

SteveTownsend commented 2 years ago

The patcher is updated at v1.3.0, I won't have time to test properly tonight but it should be fine... I hope

SlavicPotato commented 2 years ago

Just tested, looks good. The animated dawnbreaker and all.

SteveTownsend commented 2 years ago

wow that's great. Thanks for doing all the work.

On Wed, Mar 9, 2022 at 5:28 PM SlavicPotato @.***> wrote:

Just tested, looks good. The animated dawnbreaker and all.

— Reply to this email directly, view it on GitHub https://github.com/SteveTownsend/ImmersiveEquipmentMeshGen/pull/7#issuecomment-1063435605, or unsubscribe https://github.com/notifications/unsubscribe-auth/ADRDNC437W4QCNDRPYQCVNTU7EQX7ANCNFSM5QGXCZ2Q . 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>