New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Movement solver changes/fixes #1794

Open
wants to merge 22 commits into
base: master
from

Conversation

Projects
None yet
@wareya
Contributor

wareya commented Jul 6, 2018

This fixes several major problems with the current movement solver, which make it a lot harder to get stuck and much less common that you can't jump in certain locations unless you stand still.

No changelog additions yet and not ready to merge. Some of these changes can cause performance problems and bring some other bugs to attention (like capsules breaking some vanilla level design), so it shouldn't get merged until enough people look at it.

If you look at this, though, please test it, too, and compare its behavior ingame directly to current master. The problems it fixes are monumental.

This is certainly still not a perfect movement solver, but it's a lot better than what OpenMW has right now.

Problems fixed:

  • Not mapping against what's hit unless the stair code successfully runs
  • Lack of safety margin making it unsafe to do the above
  • Problem with how "don't skid up steep slopes" worked, preventing jumping while running into walls
  • Some bad interactions between jumping and stairstepping
  • Getting stuck in simple acute crevices, including some hanging sloped walls and things like the double stairs in the ald'ruhn mage's guild (complex acute crevices still get stuck)
  • The stair code freaking out in obscure cases involving steep slopes

Test cases:

  • The bottom of the stair meshes in interior vivec areas (test these with cylinders and rounded cylinders too, see actor.cpp)
  • The beds in the ald'ruhn mage's guild (their sides are steep slopes)
  • The bedside tables in the ald'ruhn mage's guild (their sides are VERY steep slopes)
  • The hanging sloped walls in the ald'ruhn mage's guild
  • The big plant pot thing near the entrance of the ald'ruhn mage's guild
  • Most walls anywhere
  • The double stairs in the ald'ruhn mage's guild (falling between the railings, not between them where you could always fit and get stuck) (note: in vanilla you can just walk between them for some reason)
  • Some of the pillars in the ald'ruhn mage's guild walls, where they intersect with the wall at an obtuse angle (as far as player collision is concerned)
  • A place in the ald'ruhn mage's guild (near a merchant) where two tall pots meet eachother at an obtuse angle (as far as player collision is concerned) (try jumping)

Things that might need adjustment:

  • ~~~There's a hack that ensures the stairs code works correctly when the player is jumping and moving upwards, but maybe the stairs code shouldn't work in this case at all.~~~
  • There's a hack that makes slopes act like walls in some cases, to keep the character from stuttering and unable to jump when like walking into the sides of beds and tables in ald'ruhn, or some rocks in the wilderness. It doesn't help in some cases.

Related forum thread (linked for bookkeeping purposes): https://forum.openmw.org/viewtopic.php?f=6&t=5200

wareya added some commits Jul 6, 2018

Movement solver changes: systematically use a safety margin, rewrite …
…the stairs code to use a safety margin, limit the tiny slope hack (sMinStep) to first iterations only, fix the ground offset, fix mapping the actor against what it collides with before sliding, fix the 'don't skid up slopes' logic, etc
account for accute crevies, fix some issues that arise when trying to…
… jump against walls that are actually just VERY steep slopes, related problems
@psi29a

This comment has been minimized.

Member

psi29a commented Jul 6, 2018

For those wanting to test along with you, can you provide save games to the places that should be tested?

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 6, 2018

Figured it would be easy to coc into those areas, but here.

saves.zip

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 6, 2018

Is there anything that can be done about the random appveyor build failures?

@psi29a

This comment has been minimized.

Member

psi29a commented Jul 7, 2018

Yeah, remove msvc2015 since it only happens with that and save us 25m build wait in the process.

@zinnschlag @AnyOldName3 @ananace you ok with that?

@ananace

This comment has been minimized.

Member

ananace commented Jul 7, 2018

I'd personally prefer not dropping MSVC2015, keeping support for the two latest versions is a good policy to have. But since it seems to be an AppVeyor logger issue - and therefore 100% out of our control - I'm not sure what we could do to avoid it.

@psi29a

This comment has been minimized.

Member

psi29a commented Jul 7, 2018

Can we set it to allow-failure (similar to how we do it for clang on linux)?

@AnyOldName3

This comment has been minimized.

Member

AnyOldName3 commented Jul 7, 2018

Has the bug been reported to AppVeyor?

wareya added some commits Jul 8, 2018

fix a big when the stair code repeatedly ran against very steep slope…
…s and ran out of iterations; and reduced likelyhood of getting too close to actors; and make the 'steep slopes are walls' hack only apply when the wall directs the player upwards
@wareya

This comment has been minimized.

Contributor

wareya commented Jul 11, 2018

Oops, I can't leave the ground while levitating. Guess I broke one of the flying checks.

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 11, 2018

Fixed.

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 12, 2018

About this hack:

There's a hack that ensures the stairs code works correctly when the player is jumping and moving upwards, but maybe the stairs code shouldn't work in this case at all.

Vanilla does something similar, so this should stay in. My worry was that it might not, since some games don't.

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 21, 2018

Alright, I can't see any more major changes needed based on my own testing. Anyone want to try to break it ingame before I change the "request for comments" to "ready for review"?

@GIuka

This comment has been minimized.

GIuka commented Jul 21, 2018

What is the benefit to snapping to surfaces when the player is moving upwards? An issue this creates is that the player can jump against a rope fence with ~300 acrobatics and then immediately snap onto it, losing all momentum. Sorry if this has an obvious answer.

Players with high acrobatics can also jump beneath awnings and other overhead geometry and will maintain all upward momentum. This can cause all sorts of weirdness. I know this has been an issue in master for years so this might not be entirely relevant to your PR but it might be worth looking into while you're working in this area.

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 21, 2018

The reason is that vanilla Morrowind does the same thing. That said, removing it is only a few lines of changes.

EDIT: This is also true of jumping into ceilings etc. from below and maintaining your vertical momentum. In morrowind's movement mechanics, air inertia is a fixed value, and doesn't change when you bounce into/slide across obstacles.

@GIuka

This comment has been minimized.

GIuka commented Jul 21, 2018

I guess I just spent so much time away from the original that I forgot how it controlled. I can see value in recreating the original physics. Your way is probably for the best since people might be expecting that behavior to carry over to OpenMW and personal preferences could be better met following the great dehardcoding.

@wareya

This comment has been minimized.

Contributor

wareya commented Jul 21, 2018

These parts of the engine are going to be the hardest to dehardcode, so they should be as readable as possible.

@akortunov

This comment has been minimized.

Contributor

akortunov commented Oct 16, 2018

Which ones?

Just ahead of player. It is an Ebonheart harbor, by the way.

@akortunov

This comment has been minimized.

Contributor

akortunov commented Oct 16, 2018

As for vanilla engine, we can not trust the TCL output - it does not render actual collisions here.

@Capostrophic

This comment has been minimized.

Contributor

Capostrophic commented Oct 16, 2018

Meh. TCB being unreliable is unsurprising.

The performance seems to be pretty good.

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 16, 2018

The situation with ebonheart stairs is weird - they're a single mesh ramp going up to a flat surface, still part of the same mesh, with exact connection and it's still making something mess up. I don't get it.

It doesn't happen with btBoxShape. Or a btConvexHullShape containing the equivalent of a btBoxShape.

Maybe the extremely large size of the triangles of the mesh is messing up some of bullet's internal math somewhere.

Bingo: https://streamable.com/dzgth (still uploading at time posted) it stops screwing up if I set the scale to 0.25 or lower, and if I set it to 0.5, you can see that the actor is getting placed inside of it even though it's overlapping it there.

...back to AABBs, or...?

PS: I don't know what the right move is for mudcrabs. Should the hull be forcibly centered (I assume it has a non-centered positioning and that's what's at fault)? Should it be let to rotate even though it'll cause problems? Should it have the x and y widths averaged together? I don't know.

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 17, 2018

Okay, my plan for mudcrabs:

Average the horizontal dimensions of the AABB together, forcibly center the scaled transform in updateCollisionObjectPosition(). This is probably a bad idea! It might break some kind of mod that, like, has a dummy actor with a very long (horizontally) AABB. What should I actually do here? Is there a way to "detect" dummy actors?

My plan for the weird collision glitch akortunov noticed:

Just use AABBs.

@psi29a

This comment has been minimized.

Member

psi29a commented Oct 26, 2018

@wareya anyway to do bound checking for those 'very long' AABBs?

Mods that do weird crap are probably just leaning into Morrowind's engine idiosyncrasies anyway.

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 26, 2018

What kind of bound checking? Do you mean like, if the AABB is 2x1, make a 2x2 AABB? Or do you mean something related to #1980 ?

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 26, 2018

PS: I will resolve the new merge conflict soon.

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 26, 2018

Morrowind has a behavior where, if overlapping an actor, you and them act like cylinders if you're on the ground, or if you're in the air and not falling. But if you're in the air and falling, then it pushes you outside of the actor, like OpenMW used to does without this PR when you stand on top of them.

I implemented the cylinder-like-overlapping-actor-collisions part in this PR, but not the pushing-outside-of-if-falling part, because it needs trace.cpp to know whether the currently-being-thrown actor is onGround or not (checking the vertical velocity alone will have false positive in some situations).

Also, this PR probably breaks the fix for Bug #2274 unless the platform moves extremely slowly.

@Capostrophic

This comment has been minimized.

Contributor

Capostrophic commented Oct 26, 2018

Also, this PR probably breaks the fix for Bug #2274 unless the platform moves extremely slowly.

You keep saying that (I remember you've said something similar before) but I don't understand what would be the problem because the fix is not physics-related at all, it simply applies the platform position change to the position of actors that are standing on the platform.

With the latest changes mRotationallyInvariant boolean is redundant.

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 26, 2018

@psi29a

This comment has been minimized.

Member

psi29a commented Oct 26, 2018

So this is still [WIP] or [More Testing Needed] ?

@wareya

This comment has been minimized.

Contributor

wareya commented Oct 26, 2018

@psi29a psi29a requested review from AnyOldName3 and zinnschlag Oct 26, 2018

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 8, 2018

I'm getting that ebonheart phantom drop issue with AABBs, too, on the vivec cantinas. Might need to make a change to the way that falling/landing is detected. As far as I can tell it happens in a span of one or zero frames, and of course there's no distance, so it should be possible to detect.

edit: Again, this only happens when walking on EXTREMELY large triangles, and they also happen to be perfectly flat, so I'm almost positive it's not a problem with the movement solver itself, but rather bullet and very large meshes.

https://streamable.com/gto0p

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 8, 2018

Also, after merging in master on a private branch, I got the recast navigation stuff and toggleactorpaths renders the actors as cylinders. Is this how the navmesh generator sees actors? Should I figure out how to change it to AABBs?
screenshot294

@psi29a

This comment has been minimized.

Member

psi29a commented Nov 8, 2018

Paging Dr. @elsid :)

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 8, 2018

Sheeeesh, this is so bad! How do I build bullet myself in use-double-precision-floats mode so I can see if it's the problem?

https://streamable.com/wyel7

@AnyOldName3

This comment has been minimized.

Member

AnyOldName3 commented Nov 8, 2018

There's a CMake option USE_DOUBLE_PRECISION, but their documentation recommends you use Premake instead of CMake, and I've no idea how you set options with that. Looking through their CMake, I think you're supposed to build Bullet 2 by setting BUILD_BULLET3 to OFF, so I assume there's an equivalent Premake option. Their build documentation seems focused on PyBullet rather than regular C/C++ Bullet, too, so there's that to add to the confusion.

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 9, 2018

I finally managed to get bullet with double-precision floats compiled and working and linked into OpenMW

  • It fixed the weird physics glitches on meshes with giant triangles e.g. in vivec and ebonheart
  • It broke the tcb command in exteriors for some reason
  • Now it freezes eventually in exteriors (after a minute or so) (interiors seem fine?)
  • It was really annoying to get bullet to compile correctly, really

Bullet was compiled with:

cmake ../ -LA -G "Visual Studio 14 2015 Win64" -DCMAKE_BUILD_TYPE=Release -DUSE_DOUBLE_PRECISION=ON -DBUILD_CPU_DEMOS=OFF -DBUILD_PYBULLET=OFF -DBUILD_BULLET2_DEMOS=OFF -DBUILD_OPENGL3_DEMOS=OFF -DBUILD_BULLET3=OFF -DUSE_MSVC_RUNTIME_LIBRARY_DLL=ON
/c/Program\ Files\ \(x86\)/MSBuild/14.0/Bin/MSBuild.exe ALL_BUILD.vcxproj -p:Configuration=Release;Platform=x64

Made the following changes to OpenMW (after merging in master for recastnavigation):

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 606d27bfd..4a83c1a5c 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -210,6 +210,8 @@ if (USE_SYSTEM_TINYXML)
     include_directories(SYSTEM ${TinyXML_INCLUDE_DIRS})
 endif()
 
+add_definitions(-DBT_USE_DOUBLE_PRECISION)
+
 # Platform specific
 if (WIN32)
     if(NOT MINGW)
diff --git a/components/detournavigator/recastmeshbuilder.cpp b/components/detournavigator/recastmeshbuilder.cpp
index e325b7eaf..bf6dc47c2 100644
--- a/components/detournavigator/recastmeshbuilder.cpp
+++ b/components/detournavigator/recastmeshbuilder.cpp
@@ -141,28 +141,28 @@ namespace DetourNavigator
         aabbMin = transform(aabbMin);
         aabbMax = transform(aabbMax);
 
-        aabbMin.setX(std::max(mBounds.mMin.x(), aabbMin.x()));
-        aabbMin.setX(std::min(mBounds.mMax.x(), aabbMin.x()));
-        aabbMin.setY(std::max(mBounds.mMin.y(), aabbMin.y()));
-        aabbMin.setY(std::min(mBounds.mMax.y(), aabbMin.y()));
+        aabbMin.setX(std::max((double)mBounds.mMin.x(), aabbMin.x()));
+        aabbMin.setX(std::min((double)mBounds.mMax.x(), aabbMin.x()));
+        aabbMin.setY(std::max((double)mBounds.mMin.y(), aabbMin.y()));
+        aabbMin.setY(std::min((double)mBounds.mMax.y(), aabbMin.y()));
 
-        aabbMax.setX(std::max(mBounds.mMin.x(), aabbMax.x()));
-        aabbMax.setX(std::min(mBounds.mMax.x(), aabbMax.x()));
-        aabbMax.setY(std::max(mBounds.mMin.y(), aabbMax.y()));
-        aabbMax.setY(std::min(mBounds.mMax.y(), aabbMax.y()));
+        aabbMax.setX(std::max((double)mBounds.mMin.x(), aabbMax.x()));
+        aabbMax.setX(std::min((double)mBounds.mMax.x(), aabbMax.x()));
+        aabbMax.setY(std::max((double)mBounds.mMin.y(), aabbMax.y()));
+        aabbMax.setY(std::min((double)mBounds.mMax.y(), aabbMax.y()));
 
         const auto inversedTransform = transform.inverse();
 
         aabbMin = inversedTransform(aabbMin);
         aabbMax = inversedTransform(aabbMax);
 
-        aabbMin.setX(std::min(aabbMin.x(), aabbMax.x()));
-        aabbMin.setY(std::min(aabbMin.y(), aabbMax.y()));
-        aabbMin.setZ(std::min(aabbMin.z(), aabbMax.z()));
+        aabbMin.setX(std::min((double)aabbMin.x(), aabbMax.x()));
+        aabbMin.setY(std::min((double)aabbMin.y(), aabbMax.y()));
+        aabbMin.setZ(std::min((double)aabbMin.z(), aabbMax.z()));
 
-        aabbMax.setX(std::max(aabbMin.x(), aabbMax.x()));
-        aabbMax.setY(std::max(aabbMin.y(), aabbMax.y()));
-        aabbMax.setZ(std::max(aabbMin.z(), aabbMax.z()));
+        aabbMax.setX(std::max((double)aabbMin.x(), aabbMax.x()));
+        aabbMax.setY(std::max((double)aabbMin.y(), aabbMax.y()));
+        aabbMax.setZ(std::max((double)aabbMin.z(), aabbMax.z()));
 
         shape.processAllTriangles(&callback, aabbMin, aabbMax);
     }
@psi29a

This comment has been minimized.

Member

psi29a commented Nov 9, 2018

To make things interesting... Debian ships both float32 and float64 libs (this holds true for Ubuntu as well).

https://packages.debian.org/sid/amd64/libbullet2.87/filelist

/usr/lib/x86_64-linux-gnu/libBullet3Collision-float64.so.2.87
/usr/lib/x86_64-linux-gnu/libBullet3Collision.so.2.87

Do we want to allow developers to chose between them or always prefer float64?

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 9, 2018

It might need some code changes. Like I said, when I did it myself it caused the game to freeze eventually, and I don't know why.

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 9, 2018

I think the freeze might be related to the recastnavigation stuff not playing well with double-precision bullet, possibly because more changes have to be made than the little diff I posted up there that gets it compiling.

Just merging in master to this branch, still using single-precision bullet:

screenshot299

Navmeshes are coherent everywhere. (Ignore how messy it is, it's mostly just zfighting and stuff.)

Merging in master to this branch AND using double-precision bullet:

screenshot296

Navmesh is cut off in weird places for no apparent reason.

@wareya

This comment has been minimized.

Contributor

wareya commented Nov 12, 2018

If you merge master into this you'll notice actors "tripping" when stepping down at the ends of staircases in Vivec or when stepping down from certain objects. That's because OpenMW (neither master nor the PR) does this weird "looney-tunes physics" thing that vanilla does, where if you walk off of steep things instead of ledges, it pretends you never left the ground (except for the fact that you can't jump): https://streamable.com/u0ydm

Right now master avoids this for tiny slope-shaped steps because of the capsule shape.

@elsid

This comment has been minimized.

Contributor

elsid commented Nov 13, 2018

I got the recast navigation stuff and toggleactorpaths renders the actors as cylinders. Is this how the navmesh generator sees actors?

Recastnavigation supports only cylinders for actors so the navmesh generator.

@elsid

This comment has been minimized.

Contributor

elsid commented Nov 13, 2018

Navmesh is cut off in weird places for no apparent reason.

There are could be a lot of reasons for this. Need to debug this branch. One way is to look how is geometry for recast is generated. There is an option to enable geometry dump into .obj format and which could be opened by recast demo. It shows geometry and allows to build navmesh.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment