Bug #4241

Everything about development and the OpenMW source code.
thegriglat
Posts: 25
Joined: 12 Jan 2015, 16:22

Bug #4241

Post by thegriglat »

Hi all,
today I have found a bug with a camera (https://bugs.openmw.org/issues/4241).
I'm not a C++ and/or OpenMW code guru and after playing with camera.cpp I found a solution which seems to be workaround (lift camera up to 2 points(pixels?) -- it is enough to "fix" issue):

Code: Select all

diff --git a/apps/openmw/mwrender/camera.cpp b/apps/openmw/mwrender/camera.cpp
index cb6188a54..8a6907de7 100644
--- a/apps/openmw/mwrender/camera.cpp
+++ b/apps/openmw/mwrender/camera.cpp
@@ -98,6 +98,9 @@ namespace MWRender
         osg::Vec3d position = worldMat.getTrans();
         if (!isFirstPerson())
             position.z() += mHeight * mHeightScale;
+        else
+            // lift camera up
+            position.z() += 2.;
         return position;
     }
Also this workaround does not work if you change screen resolution in-game but will work again after game restart.

Could you please help -- where is defined camera z() position in first person view? I tried to change almost all members in camera.cpp but without result :? I found mFirstPersonOffset, setFirstPersonOffset() and setOffset(osg::Vec3d) in npcanimation (from setSneakOffset in camera.cpp) but I'm a bit worry to touch them ...
I'm not pretending to merge this workaround -- it is more for me and better understanding of OpenMW code ;)
Thanks!
User avatar
drummyfish
Posts: 154
Joined: 22 Oct 2017, 10:13
Contact:

Re: Bug #4241

Post by drummyfish »

thegriglat wrote: 04 Dec 2017, 02:56 I'm not a C++ and/or OpenMW code guru
Hey, I'm no guru either, I've been here for about two months, but if you keep digging for long enough, you eventually find the source of the bug.

I've noticed before by writing out the camera position that if you rotate the camera, its position slightly changes, which is weird. This is a situation in which it actually shows. It's like the camera is rotating around a pivot point that's sligtly "behind" it for some reason, I don't know why that is. I think we should make the camera rotate without changing position to fix this, not change its height by some magic value (if that is what you meant by increasing the z coordinate). I'm gonna take a look at this in a minute.
thegriglat wrote: 04 Dec 2017, 02:56 but I'm a bit worry to touch them ...
I'm not pretending to merge this workaround -- it is more for me and better understanding of OpenMW code ;)
You can make an incomplete pull request at GitHub and the experienced programmers will help you out and you'll very quickly get into it. They won't blindly merge anything, don't worry, and even if you break something, we can almost always revert the changes, that's what git is for =) So I'd say don't be afraid.

UPDATE: Here I'll keep my code digging notes:

- camera position is set depending on what getFocalPoint() returns
- getFocalPoint() returns position of mTrackingNode (helper node made solely for this purpose I guess)
- Looking at processViewChange() mTrackingNode for first person is set to some node of the character model: either "Camera" or "Head" if it's not found. I checked ingame and it's the "Head" node because "Camera" doesn't exist in the model. So the camera position is the head position, which I suppose is somehow animated when you're turning around. I don't know what else to attach the camera to though. Maybe we could create the "Camera" node if it doesn't exist?
- Indeed if I attach the mTrackingNode to a MatrixTransform node I create in NpcAnimation::updateParts() as a child of the NPC root node, the camera doesn't change position when rotating. I have to check if it fixes the water bug.
- It does, however only if my new camera attaching node is straight above the player root node. However the camera has to be also shifted a little bit forward in order for the hands to look good. If I do shift forward, the problem appears again, which is weird. It seems like the player root node is somehow slightly rotating (and so rotating my node along with it).
- I think it's some of the RotateControllers, but I've never seen these so I have to study it a little =)
Chris
Posts: 1625
Joined: 04 Sep 2011, 08:33

Re: Bug #4241

Post by Chris »

It's possible that this could also be fixed by slightly raising the the wading height (how high characters can be when in water). That would need some tests to see how high vanilla lets you swim. Could also compare how vanilla works when looking up and down (use tcl to get the camera in real close to a wall, ensuring it's the same exact spot in vanilla and OpenMW, and compare how the camera moves relative to the pixels on the wall when looking up and down).
User avatar
drummyfish
Posts: 154
Joined: 22 Oct 2017, 10:13
Contact:

Re: Bug #4241

Post by drummyfish »

Chris wrote: 04 Dec 2017, 22:36 It's possible that this could also be fixed by slightly raising the the wading height
That also seems just like a workaround because you can always go below the wading height and the problem will still be there. On the other hand, if this is how vanilla works and there is no other nice solution that wouldn't involve attaching the camera to an arbitrary fixed position, there may be no other way.

UPDATE: Took some vanilla footage. It seems like it has similar problems and we may end up just increasing the wading height, BUT in OpenMW the camera can get from above water to under water by looking not only upwards but also downwards, which in vanilla doesn't happen. So I think we do still have some more fundamental bug in the head animation that needs solving.
thegriglat
Posts: 25
Joined: 12 Jan 2015, 16:22

Re: Bug #4241

Post by thegriglat »

I tried this, camera works fine but view of hands changed.

Code: Select all

diff --git a/apps/openmw/mwrender/camera.cpp b/apps/openmw/mwrender/camera.cpp
index cb6188a54..13c52492a 100644
--- a/apps/openmw/mwrender/camera.cpp
+++ b/apps/openmw/mwrender/camera.cpp
@@ -87,7 +90,7 @@ namespace MWRender
 
     osg::Vec3d Camera::getFocalPoint()
     {
-        const osg::Node* trackNode = mTrackingNode;
+        const osg::Node* trackNode = mTrackingPtr.getRefData().getBaseNode();
         if (!trackNode)
             return osg::Vec3d();
         osg::NodePathList nodepaths = trackNode->getParentalNodePaths();
@@ -96,8 +99,8 @@ namespace MWRender
         osg::Matrix worldMat = osg::computeLocalToWorld(nodepaths[0]);
 
         osg::Vec3d position = worldMat.getTrans();
-        if (!isFirstPerson())
-            position.z() += mHeight * mHeightScale;
+        position.z() += mHeight * mHeightScale;
         return position;
     }
 
Not sure why it works.
User avatar
drummyfish
Posts: 154
Joined: 22 Oct 2017, 10:13
Contact:

Re: Bug #4241

Post by drummyfish »

thegriglat wrote: 05 Dec 2017, 01:04 Not sure why it works.
Because you shifted the camera up, the height scale is there only for third person so that the camera is not centered on player's waist I guess. In first person the camera is attached to the head and so the height scale is not used. This is just another workaround, but at least we have a few workarounds to choose from now :)
thegriglat
Posts: 25
Joined: 12 Jan 2015, 16:22

Re: Bug #4241

Post by thegriglat »

I submitted PR in which I change only z-coordinate so the camera is centered as before. But yes, unlikely it is a suitable solution.
User avatar
drummyfish
Posts: 154
Joined: 22 Oct 2017, 10:13
Contact:

Re: Bug #4241

Post by drummyfish »

I got frustrated of printing out OSG values while debugging this so I started this, feel free to use. You only include the header, that's all.
User avatar
scrawl
Posts: 2152
Joined: 18 Feb 2012, 11:51

Re: Bug #4241

Post by scrawl »

You can replace the to_str stuff by including <osg/io_utils> then e.g. cout << vec3 << std::endl;. I assume this is in a separate header to not make every file depend on iostream.
User avatar
drummyfish
Posts: 154
Joined: 22 Oct 2017, 10:13
Contact:

Re: Bug #4241

Post by drummyfish »

scrawl wrote: 05 Dec 2017, 23:02 You can replace the to_str stuff by including <osg/io_utils> then e.g. cout << vec3 << std::endl;. I assume this is in a separate header to not make every file depend on iostream.
Nice, I thought there had to be something like this. I like my lib though because for example I try to output the stuff in readable way, e.g. floating point always in fixed format (scientific notation makes a bunch of numbers really unreadable), printing rotations as Euler angles in degrees (radians are well readable only when used with fractions of pi) etc. Also it's very exhausting to type std::cout << something << std::endl vs print(something).

The header is only meant to be included in the file you're debugging.
Post Reply