fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale #4897

pollend · 2021-09-07T05:57:30Z

I fixed two things, I corrected the debug display for the skeleton render and made sure to make scale and translate as replicated so this should fix the offset animated meshes in MetalRenegades.

I think we need to re-think how this works at the moment because there is a lot of back and forth between SkeletonRender <--> OpenGLSkeletalMesh <--> SkeletalMeshData. The coupling is very tight and a lot of state information is funneled between these three classes. I'm finding it hard to see how this fits together with how this is organized at the moment.

…eleton

…olve-update

skaldarnar · 2021-09-07T17:53:25Z

engine/src/main/java/org/terasology/engine/rendering/logic/SkeletonRenderer.java

+                Vector3f location = locationComponent.getWorldPosition(new Vector3f());
+                Quaternionf rotation = locationComponent.getWorldRotation(new Quaternionf());
+                Matrix4f transform = new Matrix4f().translationRotateScale(location, rotation, 1.0f);


Is there any reason to create new instances for the location vector, the rotation quaternion, and the transformation matrix with each render call for each entity?

You often raise concerns about code like this, so I'm wondering about all the instantiations in this code render code block that gets called quite often (if the debug feature is enabled).

umm, I guess I could use a temporary Matrix. this is because we have custom offset and translation for the SkeletonComponent :/

So we need individual vectors here and cannot reuse a single temporary object instance for rendering the skeleton for all entities?

nope, we can cache it, I was just getting lazy. was looking at this at 10 something.

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

…olve-update

jdrueckert · 2021-09-29T22:35:19Z

I'm not sure this works correctly yet 🤔
In-game I saw the following:

Note that the debug skeleton seems to be higher in the air than the actual model. @pollend did you only correct scaling the debug skeleton or also applying an offset?

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

pollend · 2021-10-03T14:58:41Z

@jdrueckert this fixes the offset in the overlay rig but this kind of horrible so this offset will just offset the camera space and additionally we also have an additional translation on top of the rig itself to also offset things. everything in the cameras view for the rig is offset by that amount. not sure if that lines up with world coordinates at all?

…olve-update

jdrueckert

First of all it's hard for me to review this because I don't understand the math behind this, so please add comments especially in the new code in SkeletonRenderer that explain what the respective parts do.

Second, why do we need to offset the camera to have the debug skeleton match the actual one the model is matching? This does not seem to make sense to me? Where does this mismatch between debug skeleton and model come from? Is the model not matching the skeleton or is the rendered debug skeleton not matching the "real" skeleton?

Third, there's open checkstyle warnings.

engine/src/main/java/org/terasology/engine/rendering/opengl/OpenGLSkeletalMesh.java

pollend · 2021-10-30T19:27:02Z

First of all it's hard for me to review this because I don't understand the math behind this, so please add comments especially in the new code in SkeletonRenderer that explain what the respective parts do.

Second, why do we need to offset the camera to have the debug skeleton match the actual one the model is matching? This does not seem to make sense to me? Where does this mismatch between debug skeleton and model come from? Is the model not matching the skeleton or is the rendered debug skeleton not matching the "real" skeleton?

Third, there's open checkstyle warnings.

there is some code up a couple lines I'm just duplicating that logic.

jdrueckert · 2021-10-30T20:05:22Z

First of all it's hard for me to review this because I don't understand the math behind this, so please add comments especially in the new code in SkeletonRenderer that explain what the respective parts do.
Second, why do we need to offset the camera to have the debug skeleton match the actual one the model is matching? This does not seem to make sense to me? Where does this mismatch between debug skeleton and model come from? Is the model not matching the skeleton or is the rendered debug skeleton not matching the "real" skeleton?
Third, there's open checkstyle warnings.

there is some code up a couple lines I'm just duplicating that logic.

Can you please note on which of my three points you're talking about and reference specific lines?

pollend · 2021-10-30T22:38:35Z

looks like I applied heightoffset twice

…olve-update

jdrueckert

Tests out fine in-game now, but I think we can improve on documentation here while we're at it 🙏

jdrueckert · 2021-11-02T19:19:45Z

engine/src/main/java/org/terasology/engine/rendering/logic/SkeletonRenderer.java

+                Vector3f worldPos = new Vector3f();
+                Vector3f worldPositionCameraSpace = new Vector3f();
+                worldPos.sub(cameraPosition, worldPositionCameraSpace);


This seems a bit lengthy for

Suggested change

Vector3f worldPos = new Vector3f();

Vector3f worldPositionCameraSpace = new Vector3f();

worldPos.sub(cameraPosition, worldPositionCameraSpace);

Vector3f worldPositionCameraSpace = cameraPosition.negate(new Vector3f());

jdrueckert · 2021-11-02T19:21:50Z

engine/src/main/java/org/terasology/engine/rendering/logic/SkeletonRenderer.java

+                Vector3f location = locationComponent.getWorldPosition(new Vector3f());
+                Quaternionf rotation = locationComponent.getWorldRotation(new Quaternionf());
+                entityTransform.translationRotateScale(location, rotation, 1.0f);
+
+                Vector3f worldPos = new Vector3f();
+                Vector3f worldPositionCameraSpace = new Vector3f();
+                worldPos.sub(cameraPosition, worldPositionCameraSpace);
+
+                // same heightOffset is applied to worldPositionCameraSpace from #renderOpaque()
+                // TODO: resolve repeated logic for transformation applied to bones
+                worldPositionCameraSpace.y += skeletalMesh.heightOffset;
+
+                Matrix4f matrixCameraSpace = new Matrix4f().translationRotateScale(worldPositionCameraSpace, new Quaternionf(), 1.0f);
+                Matrix4f modelViewMatrix = new Matrix4f(worldRenderer.getActiveCamera().getViewMatrix()).mul(matrixCameraSpace);
+                material.setMatrix4("projectionMatrix", worldRenderer.getActiveCamera().getProjectionMatrix());
+                material.setMatrix4("modelViewMatrix", modelViewMatrix, true);


As somebody that never really paid a lot of attention in my university computer graphics course, this is all Greek to me... can you please add some comments explaining what's happening here and why?

jdrueckert · 2021-11-02T19:31:53Z

engine/src/main/java/org/terasology/engine/rendering/logic/SkeletonRenderer.java

+                    locCompA.getRelativeTransform(relMat.identity(), entity);
+                    result.set(entityTransform)
+                            .mul(relFinal.identity()
+                                    .scale(skeletalMesh.scale)
+                                    .translate(skeletalMesh.translate)
+                                    .mul(relMat))
+                            .transformPosition(currentPos.zero());
+                    meshData.position.put(currentPos);
+
+
+                    locCompB.getRelativeTransform(relMat.identity(), entity);
+                    result.set(entityTransform)
+                            .mul(relFinal
+                                    .identity()
+                                    .scale(skeletalMesh.scale)
+                                    .translate(skeletalMesh.translate)
+                                    .mul(relMat))
+                            .transformPosition(currentPos.zero());
+                    meshData.position.put(currentPos);


same here pls

…olve-update

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

jdrueckert

Thanks a lot for the improved code comments! They are way more helpful!

jdrueckert · 2021-11-14T22:18:37Z

engine/src/main/java/org/terasology/engine/rendering/logic/SkeletonRenderer.java

                    locCompA.getRelativeTransform(relMat.identity(), entity);
+                    // entityTransform * (scale, translation) * relativeMat * [x,y,z,1]
                    result.set(entityTransform)
                            .mul(relFinal.identity()
                                    .scale(skeletalMesh.scale)
                                    .translate(skeletalMesh.translate)
-                                    .mul(relMat)) // apply relative transformation for the skeletalMesh scale and transform
-                            .transformPosition(currentPos.zero());
-                    meshData.position.put(currentPos);
+                                    .mul(relMat))
+                            .transformPosition(currentPos.zero()); // get the position of the start of the bone
+                    meshData.position.put(currentPos); // the start of the bone

-                    // relative transform around the root skeletalMesh.scale and skeletalMesh.translate are relative to root
+                    // need to calculate the relative transformation from the entity to the connecting bone
                    locCompB.getRelativeTransform(relMat.identity(), entity);
+                    // entityTransform * (scale, translation) * relativeMat * [x,y,z,1]
                    result.set(entityTransform)
                            .mul(relFinal
                                    .identity()
                                    .scale(skeletalMesh.scale)
                                    .translate(skeletalMesh.translate)
-                                    .mul(relMat))  // apply relative transformation for the skeletalMesh scale and transform
-                            .transformPosition(currentPos.zero());
-                    meshData.position.put(currentPos);
+                                    .mul(relMat))
+                            .transformPosition(currentPos.zero()); // get the position to the connecting bone
+                    meshData.position.put(currentPos); // the end of the bone


Those two sections seem to do pretty much the same apart from the difference between locCompA and locCompB which according to the doc comments are the start of the bone vs the connecting bone.
Do you think it would make sense to extract this into a separate function and call it with the respective locComp?

Would require passing a lot of parameters. Will be overhauled by @pollend soon-ish anyway, at which point we should ensure a comprehensible structure of the logic. Accepting it as is for now.

…olve-update

bugfix: resolve SkeletonRenderer for multiplayer and correct debug sk…

3f181eb

…eleton

pollend requested review from skaldarnar and jdrueckert September 7, 2021 05:57

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

2d59d7a

…olve-update

github-actions bot added the Type: Bug Issues reporting and PRs fixing problems label Sep 7, 2021

skaldarnar reviewed Sep 7, 2021

View reviewed changes

skaldarnar added the Topic: Rendering Requests, Issues and Changes related to lighting, meshes, camera, etc. label Sep 7, 2021

skaldarnar changed the title ~~bugfix: resolve SkeletonRenderer for multiplayer and correct debug skeleton~~ fix(rendering): replicate SkeletalMesh fields and correctly scale debug skeleton Sep 7, 2021

pollend added 2 commits September 7, 2021 20:04

chore: cache matrix and vector

5c86c10

Merge branch 'bugfix/correct-SkeletonRenderer-debug-resolve-update' o…

70b323b

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

pollend requested a review from skaldarnar September 9, 2021 04:33

pollend and others added 4 commits September 9, 2021 19:23

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

19ed92d

…olve-update

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

54496d9

…olve-update

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

99f7015

…olve-update

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

6a49a99

…olve-update

pollend added 2 commits October 3, 2021 07:55

feat: add offset to camera

a1d425c

Merge branch 'bugfix/correct-SkeletonRenderer-debug-resolve-update' o…

582e579

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

b6d94af

…olve-update

jdrueckert requested changes Oct 30, 2021

View reviewed changes

engine/src/main/java/org/terasology/engine/rendering/opengl/OpenGLSkeletalMesh.java Show resolved Hide resolved

chore: update renderoverlay

8cee37c

pollend and others added 3 commits October 30, 2021 15:44

chore: add todo

fe25375

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

be929fc

…olve-update

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

75e09b6

…olve-update

jdrueckert requested changes Nov 2, 2021

View reviewed changes

chore: add comments to SkeletonRenderer

c8d5c6d

pollend requested a review from jdrueckert November 7, 2021 18:49

pollend and others added 3 commits November 7, 2021 17:00

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

171a1ad

…olve-update

chore: update docs

492e81d

Merge branch 'bugfix/correct-SkeletonRenderer-debug-resolve-update' o…

783e162

…f github.com:MovingBlocks/Terasology into bugfix/correct-SkeletonRenderer-debug-resolve-update

jdrueckert reviewed Nov 14, 2021

View reviewed changes

Merge branch 'develop' into bugfix/correct-SkeletonRenderer-debug-res…

60668de

…olve-update

jdrueckert approved these changes Nov 14, 2021

View reviewed changes

jdrueckert changed the title ~~fix(rendering): replicate SkeletalMesh fields and correctly scale debug skeleton~~ fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale Nov 14, 2021

jdrueckert merged commit f105255 into develop Nov 14, 2021

jdrueckert deleted the bugfix/correct-SkeletonRenderer-debug-resolve-update branch November 14, 2021 22:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale #4897

fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale #4897

pollend commented Sep 7, 2021

skaldarnar Sep 7, 2021

pollend Sep 7, 2021

skaldarnar Sep 7, 2021

pollend Sep 7, 2021

jdrueckert commented Sep 29, 2021 •

edited

Loading

pollend commented Oct 3, 2021 •

edited

Loading

jdrueckert left a comment

pollend commented Oct 30, 2021

jdrueckert commented Oct 30, 2021

pollend commented Oct 30, 2021

jdrueckert left a comment

jdrueckert Nov 2, 2021

jdrueckert Nov 2, 2021

jdrueckert Nov 2, 2021

jdrueckert left a comment

jdrueckert Nov 14, 2021

jdrueckert Nov 14, 2021

fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale #4897

fix(rendering): replicate SkeletalMesh fields, fix debug skeleton scale #4897

Conversation

pollend commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Sep 29, 2021 • edited Loading

pollend commented Oct 3, 2021 • edited Loading

jdrueckert left a comment

Choose a reason for hiding this comment

pollend commented Oct 30, 2021

jdrueckert commented Oct 30, 2021

pollend commented Oct 30, 2021

jdrueckert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdrueckert commented Sep 29, 2021 •

edited

Loading

pollend commented Oct 3, 2021 •

edited

Loading