ousnius / BodySlide-and-Outfit-Studio

BodySlide and Outfit Studio, a tool to convert, create, and customize outfits and bodies for Bethesda games.
GNU General Public License v3.0
293 stars 64 forks source link

Cleaned up mesh coordinate transforms #425

Closed sts1skj closed 2 years ago

sts1skj commented 2 years ago

This commit is mostly cosmetic changes to improve clarity. But it also fixes bugs in computing mesh::matModel. matModel is computed in five different places, and each had at least one bug. In fixing these bugs, I deleted the function mesh::TransformToMatrix4, which definitely had a rotation bug, and possibly a translation bug depending on how it was called. Different callers actually needed different functionality. So I implemented correct functionality in the five callers rather than have them call a common function that needed to be different for each of them.

I added static variables xformNifToMesh and xformMeshToNif to mesh. These encode, as a MatTransform, the transformation that multiplies or divides by 10, negates x, and swaps y and z.

I added member variables xformMeshToModel and xformModelToMesh to mesh. These keep the MatTransforms corresponding to matModel and its inverse so they can be used directly instead of having to convert nifly::Vector3 to glm::vec4 and back. I added setup functions for these three member variables.

I added static functions to mesh that do the same thing as VecToMeshCoords and VecToNifCoords, but for direction vectors, difference vectors, and distances in addition to position vectors.

I added member functions to mesh that apply xformMeshToModel and xformModelToMesh to positions, directions, differences, and distances.

I tried my best to use these new mesh functions wherever appropriate. I searched for all instances of '10.0f', '* 10', and '/ 10' in src to try to catch places where VecToMeshCoords and VecToNifCoords should have been called but weren't.

mesh::ApplyMatrix4 is no longer used anywhere, but I left it in in case someone has a use for it someday.

sts1skj commented 2 years ago

While this pull request does not fix the automorpher so that it can conform sliders on shapes with complex global-to-skin or global-to-node transforms, it does fix one bug relating to that. Now, correct coordinates are passed to the kd_tree code so that point proximities are correctly determined. The diffs generated are still bad, but at least the right vertices have diffs.

sts1skj commented 2 years ago

In Automorph::MeshFromNifShape, a lot of effort is made to compute matModel (and now xformMeshToModel and xformModelToMesh) for the mesh, but at the end of the function the points are transformed into model space (which is the same as nif global space), so the matModel stored in the mesh is no longer correct; at the end of the function, mesh space is the same as model space, so the transforms should be the identity. But, as far as I can tell, matModel (and xformMeshToModel and xformModelToMesh) are not used outside of this function, so the inconsistency is harmless.

While this is not a bug, I find it annoying and distasteful. I would fix this by not setting matModel (and xformMeshToModel and xformModelToMesh) at all, and just applying the computed transform directly. May I?

sts1skj commented 2 years ago

I noticed that the function OutfitProject::ApplyTransformToShapeGeometry does not apply the transform to position slider diffs. I think it probably should.

sts1skj commented 2 years ago

There are probably a bunch of places in TweakBrush.cpp where any scale difference between mesh and model coordinates should be taken into consideration. One example is deciding which points are within the brush radius. Another would be applying brush strength, but only for brushes where strength determines the size of motion.

sts1skj commented 2 years ago

I would like to rename: VecToMeshCoords -> PosNifToMesh VecToNifCoords -> PosMeshToNif to be consistent with the other transform functions I added in this update. Or, if you prefer, I could rename all the new functions. May I rename these two functions?

sts1skj commented 2 years ago

The last commit fixes the automorpher (my first comment above). I used the mesh transforms, so cancel my second comment above (about discarding the mesh-to-model transform in Automorph::MeshFromNifShape).

ousnius commented 2 years ago

I would like to rename: VecToMeshCoords -> PosNifToMesh VecToNifCoords -> PosMeshToNif to be consistent with the other transform functions I added in this update. Or, if you prefer, I could rename all the new functions. May I rename these two functions?

Feel free to rename the two functions. Maybe adding a verb would be even better, though. For example: ConvertPosNifToMesh and ConvertPosMeshToNif or something similar if you can think of a better one.

sts1skj commented 2 years ago

How about TransformPosNifToMesh and TransformPosMeshToNif?

ousnius commented 2 years ago

How about TransformPosNifToMesh and TransformPosMeshToNif?

Sounds good.

sts1skj commented 2 years ago

Should the x-mirror tool option mirror in mesh coordinates or model coordinates? Currently, all the tools mirror in mesh coordinates, not model coordinates.

sts1skj commented 2 years ago

I've run out of tasks for this pull request, for the moment. It's likely I've missed something, but I may have caught every instance in Outfit Studio where non-identity model-to-mesh transforms were not handled correctly. (The model-to-mesh transform is generally not the identity when the nif's global-to-skin or node-to-global transform is not the identity.)

I could change the tools so that x-mirror happens in mesh space instead of model space, but I like it better with x-mirror happening in model space.

ousnius commented 2 years ago

Tested with various meshes.