Valve 220 texture lock fix #1458

ericwa · 2016-10-02T05:08:44Z

Fix for Texture lock broken in Valve 220 format #1454 . I reverted 36e49c4 and now I just bail out at the beginning of ParallelTexCoordSystem::doTransform if lockTexture is false. Texture lock OFF and ON both seem to work now.
Tests for texture lock. The testTextureLock_Parallel fails without the fix to ParallelTexCoordSystem.
~~There are no tests yet for the case when texture lock is off~~ There is a basic test of "texture lock off" now. It only tests the case when a face has reset texture alignment before being transfomed.

…tem." This reverts commit 36e49c4.

…lTexCoordSystem) without breaking the texture lock "on" case.

ericwa · 2016-10-02T05:14:17Z

Hm.. just noticed a problem, rotating a brush with texture lock OFF stretches the texture. So I guess ParallelTexCoordSystem::doTransform needs to do some more work if lockTexture is false.

Reduce excessive number of translation test cases in checkTextureLockWithTranslationAnd90DegreeRotations because it was slow

ericwa · 2016-10-02T07:10:51Z

All of my tests pass except for the ones testing ParallelTexCoordSystem with texture lock off.

I'm not sure how to fix ParallelTexCoordSystem::doTransform for the case when texture lock is off and the user does a rotation. That's a somewhat niche use case though; everything else seems to be working.

kduske · 2016-10-02T07:24:21Z

What is the problem? Why is rotation special?

ericwa · 2016-10-02T16:55:23Z

The problem looks like this: https://www.dropbox.com/s/50ayw9lqn1i8mgk/rotate.mov?dl=0
I guess rotation is special because, with translation, you can get away with leaving the texture axes alone. If you don't touch the texture axes, but rotate a face 90 degrees, you get the stretched look seen in the video.

kduske · 2016-10-02T18:00:59Z

Is it not possible to compensate for the rotation? It sounds like you need to apply rotation to the texture axes in that case, essentially behaving as if texture lock was in, but undoing the translation?

kduske · 2016-10-02T19:06:26Z

Thanks for the video. I think it makes sense that the texture coordinate system must be rotated even if texture lock is off. So that would mean that texture lock only applies to translation and nothing else in Valve 220.

Do you know how other editors handle this?

ericwa · 2016-10-02T19:14:34Z

I just checked Sledge, it does what you said: "texture lock off" only applies to translation. Rotation always gets texture lock.

Here is Sledge's texture lock code:
https://github.com/LogicAndTrick/sledge/blob/master/Sledge.DataStructures/MapObjects/Face.cs#L405

I'll see if I can get "texture lock off" only for translation to work.

kduske · 2016-10-02T19:29:38Z

Remember that you can very easily separate the translational component from a transformation matrix, as it is stored independently from the rest in the fourth column (or the fourth row?). That way, you can separate them (there should be methods for that already) and apply only the non-translational part to the texture axes.

You may also have to update the stupid angle value somehow. It represents by how much a texture coordinate system has been rotated away from its default position. It's solely used by the UI and it's utterly stupid.

kduske · 2016-10-02T19:31:40Z

See methods translationMatrix(const Mat<T,S,S>&) and stripTranslation(const Mat<T,S,S>&) in Mat.h.

…compensate for the translation part of the transformation. Still compensate for the rotation.

ericwa · 2016-10-02T20:31:01Z

Ok, using stripTranslation seems to work. The stretching when rotating is fixed.

Only remaining concern I have is, will removing the attribs.xScale() == 0.0f || attribs.yScale() == 0.0f check cause problems? is that just there to avoid an assertion failure later in the method?

… used, which could lead to tests incorrectly passing because the tests were making translations of integer units.

kduske · 2016-10-02T21:06:27Z

Those checks are important, because there is a bug elsewhere that sometimes produces 0 scaling factors, which then cause all sorts of havoc.

…in ParallelTexCoordSystem::doTransform

ericwa · 2016-10-02T21:27:53Z

Ok, I just restored those checks. I'm assuming there is no need to update the texture axes in that case, since the texture alignment is going to be nonsense anyway?

kduske · 2016-10-02T21:29:08Z

Yes. I think I should add assertions to the setters of those attributes to catch the bug earlier.

kduske

I'll merge and make the requested changes myself. Thanks for your help!

kduske · 2016-10-02T21:35:53Z

common/src/Model/ParallelTexCoordSystem.cpp

@@ -102,18 +102,14 @@ namespace TrenchBroom {
            m_yAxis = rot * m_yAxis;
        }

-        void ParallelTexCoordSystem::doTransform(const Plane3& oldBoundary, const Mat4x4& transformation, BrushFaceAttributes& attribs, bool lockTexture, const Vec3& oldInvariant) {
+        void ParallelTexCoordSystem::doTransform(const Plane3& oldBoundary, const Mat4x4& origTransformation, BrushFaceAttributes& attribs, bool lockTexture, const Vec3& oldInvariant) {


Please don't use abbreviated variable names. Also I think it's better to keep translation as the variable name and use a different name below.

kduske · 2016-10-02T21:36:18Z

common/src/Model/ParallelTexCoordSystem.cpp

                return;

+            // when texture lock is off, don't compensate for the translation part of the transformation
+            const Mat4x4 transformation = lockTexture ? origTransformation : stripTranslation(origTransformation);


I'd prefer to use a name like actualTransformation or effectiveTransformationhere.

kduske · 2016-10-02T21:37:51Z

test/src/Model/BrushFaceTest.cpp

+            checkTextureLockOffWithTranslation(origFace);
+        }
+
+        TEST(BrushFaceTest, testTextureLock_Paraxial) {


Thanks for the test cases, looks like it was more work than the actual changes ;-)

kduske · 2016-10-02T21:38:12Z

test/src/TestUtils.cpp

                return false;
        }
        return true;
    }
+
+    TEST(TestUtilsTest, testTexCoordsEqual) {


Good man ;-)

ericwa · 2016-10-02T21:57:31Z

Happy to help!

The tests should be useful if any changes / bug fixes need to be made to the texture lock code in the future. They seem to be fairly thorough, e.g. they'll fail if some of the "magic" bits of ParaxialTexCoordSystem are commented out, like this thing

kduske · 2016-10-02T22:02:05Z

Yeah, that "magic" stuff is annoying. I wish I understood it better and that the algorithm was easier, but at some point I just gave up and declared it good enough.

ericwa added 3 commits October 1, 2016 22:32

Texture lock tests. Currently failing due to 36e49c4

3b8b209

Revert "Respect unset texture lock in parallel texture coordinate sys…

d96c04f

…tem." This reverts commit 36e49c4.

Attempt to reimplement 36e49c4 (support texture lock "off" in Paralle…

c27be91

…lTexCoordSystem) without breaking the texture lock "on" case.

Add a partial test for transforming a face with texture lock off.

50d2e7c

Reduce excessive number of translation test cases in checkTextureLockWithTranslationAnd90DegreeRotations because it was slow

ericwa added 2 commits October 2, 2016 14:24

Only test "texture lock off" with translations

dfeed68

ParallelTexCoordSystem::doTransform: when texture lock is off, don't …

a442acb

…compensate for the translation part of the transformation. Still compensate for the rotation.

Texture lock tests: use a 64x64 texture. Previously a 1x1 texture was…

4d4f0d2

… used, which could lead to tests incorrectly passing because the tests were making translations of integer units.

Restore (attribs.xScale() == 0.0f || attribs.yScale() == 0.0f) check …

eb80c41

…in ParallelTexCoordSystem::doTransform

kduske approved these changes Oct 2, 2016

View reviewed changes

kduske merged commit 73e6876 into TrenchBroom:release/v2.0.0 Oct 2, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valve 220 texture lock fix #1458

Valve 220 texture lock fix #1458

ericwa commented Oct 2, 2016 •

edited

ericwa commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske commented Oct 2, 2016 •

edited

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske left a comment

kduske Oct 2, 2016

kduske Oct 2, 2016

kduske Oct 2, 2016

kduske Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

Valve 220 texture lock fix #1458

Valve 220 texture lock fix #1458

Conversation

ericwa commented Oct 2, 2016 • edited

ericwa commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske commented Oct 2, 2016 • edited

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

kduske left a comment

Choose a reason for hiding this comment

kduske Oct 2, 2016

Choose a reason for hiding this comment

kduske Oct 2, 2016

Choose a reason for hiding this comment

kduske Oct 2, 2016

Choose a reason for hiding this comment

kduske Oct 2, 2016

Choose a reason for hiding this comment

ericwa commented Oct 2, 2016

kduske commented Oct 2, 2016

ericwa commented Oct 2, 2016 •

edited

kduske commented Oct 2, 2016 •

edited