ousnius / nifly

C++ NIF library for the Gamebryo/NetImmerse File Format
GNU General Public License v3.0
54 stars 21 forks source link

Issue handling trailing empty segments #20

Closed BadDogSkyrim closed 2 years ago

BadDogSkyrim commented 2 years ago

Hey, I have a small thing to report:

For FO4, if you define a set of, say, 7 segments and assign all the tris to the 4th (so 5-7 are empty) then nifly will assign all those tris to the last segment, not the 4th.

Arguably, this is a case where if you do stupid things, stupid things happen. I found it because all the body parts in FO4 use the same segmentation scheme: 7 segments representing nothing, head, right arm, torso, left arm, right leg, left leg, in that order. So I just defined all the segments and assigned faces as needed. Some of the armors only needed a torso segment, leaving segments 5-7 empty. Took me quite a while to figure out what was going on.

I dug into the nifly code and the issue is below, in geometry.cpp, SetSegmentation(). The original code is commented out below. It was searching for the last filled part, but the trailing empty segments were confusing it. Capturing lastFilledPart during the first loop fixed the problem. (I can submit this as a pull req if you'd prefer, but I wasn't sure you'd even consider it a real bug.)

size_t lastFilledPart = 0; for (int i = 0; i < static_cast<int>(triInds.size()); ++i) while (triParts[triInds[i]] >= j) { lastFilledPart = j; partTriInds[j++] = i; };

//for (size_t i = 0; i < partTriInds.size(); i++) // if (partTriInds[i] > 0) // lastFilledPart = i;

ousnius commented 2 years ago

@BadDogSkyrim As you can see based on the following commits, that part of the code is a bit prone to errors and regression. https://github.com/ousnius/nifly/commit/eae5755373b12b9ba24cfe2113e34cacbc9cc781 https://github.com/ousnius/nifly/commit/bb1f7a6f56f045ddb321b329a3bfc8542b5eaa92

I don't feel comfortable touching that function with a 10-foot pole without doing extensive tests with different NIF files and combinations of segments. Unfortunately nifly doesn't have much FO4 or any segmentation tests currently.

If you have the time and interest, do you think you could possibly do some testing with different ways of filling segments?

BadDogSkyrim commented 2 years ago

Sure, I can do. I've got a set of regression tests in place already--cuz I'm too much of a klutz to do this stuff without a safety net--and I can make sure your scenarios are covered. I know heads do your #4 (empty segments between used segments) bc heads are seg 1 and necks are seg 3 in the head nifs (that's 0-based).

BadDogSkyrim commented 2 years ago

I ran full regression tests on that fix I posted, covering all your scenarios, and they all check out. So I'm as confident in it as I can be.

I can share the tests, but they call in through my DLL wrapper layer so they may not be that useful to you.

ousnius commented 2 years ago

@BadDogSkyrim Thanks for testing it thoroughly. Feel free to make a PR anytime, I will do some of my own testing then as well.

ousnius commented 2 years ago

@BadDogSkyrim @sts1skj Fixed this with #26

BadDogSkyrim commented 2 years ago

Cool, thx. I had pulled in the latest changes preparatory to a PR, but I'll pull down these changes instead and run them through my test suite.

BadDogSkyrim commented 2 years ago

The new updates check out on my end.