isaac-mason / recast-navigation-js

JavaScript navigation mesh construction, path-finding, and spatial reasoning toolkit. WebAssembly port of Recast Navigation.
https://recast-navigation-js.isaacmason.com
MIT License
226 stars 18 forks source link

Obstacle queue not processing batches of updates #312

Open DVLP opened 2 months ago

DVLP commented 2 months ago

First of all thanks for making a good JS wrapper for Recast. It's the most usable one and with good examples too!

Now the issue. If I understood the system correctly tileCache.update(navMesh) needs to be called to process the queue of up to 64 added obstacles. So I wrote this code to add multiple boxes to a test level (earlier passed maxObstacles: 1000 option accordingly)

for (var i = 0; i < 1000; i++) {
      const x = Math.random() * 500 - 250
      const z = Math.random() * 500 - 250
      const y = 0
      const box = tileCache.addBoxObstacle(
        { x, y, z },
        { x: 2, y: 2, z: 2 },
        Math.random()
      )

      if (i % 64 === 0) {
        let upToDate = false
        while (!upToDate) {
          const result = tileCache.update(navMesh)
          upToDate = result.upToDate;
        }
      }
    }
}

With this code only a few boxes are seen by the navmesh helper. The rest is only visible by the TileCacheHelper with no effect on the navmesh. I updated navmesh helper after adding all the obstacles to make sure it's up to date.

Then I commented out the part that limits execution of tileCache.update(navMesh) to each 64th pass. Now it's processing the update after each call to tileCache.addBoxObstacle.

      // if (i % 64 === 0) {
        let upToDate = false
        while (!upToDate) {
          const result = tileCache.update(navMesh)
          upToDate = result.upToDate;
        }
      //}

With this code every box is now correctly seen by the navmesh and the agents. It seems like there's an issue with the queue system or I just use the library incorrectly

isaac-mason commented 2 months ago

Hey @DVLP,

It's entirely possible I've mis-documented the specifics of the obstacle queue behaviour.

I tried to be clever and add more documentation than recastnavigation itself by reading the c++ implementation.

I'll take a second pass!

isaac-mason commented 2 months ago

I've made a related change that could help here: https://github.com/isaac-mason/recast-navigation-js/commit/d490a5c34e1880c4fb4ff0b904f92d8e8bf77f00

It's possible for these APIs (addCylinderObstacle, addBoxObstacle, removeObstacle) to fail. If they do fail, the c++ DetourTileCache APIs return a status code explaining why.

This isn't currently exposed, but that change (pending release) exposes them.

DVLP commented 2 months ago

Thanks for looking into this. I'll try later to see if it actually comes back with success false when running this in a loop without updating tileCache every time

DVLP commented 2 months ago

Found some time to check this again with the lastest version.

This is a result when running navmesh.update() (recursively until it's done) on each 64th iteration when adding boxes in a loop image

It looks like the queue size is around 25-26 but that's not completely reliable. When running a full update on each 25th iteration I was still getting gaps. btw the result of addBoxObstacle is always a success.

When running on every 23rd iteration I'd still get some boxes not registered. On each 22nd or 21st I'd get some artifacts like only half of the box would become a hole in the navmesh. image

Running the update on each 20th iteration seems to be the most reliable without leaving artifacts (except of some small corners cut but that's a different issue).

btw I'm also having an issue with artifacts when moving a box around that is larger than the tiles and on one side it's not creating a hole in the tiles

DVLP commented 2 months ago

Here's my setup code

    function tileCacheFullUpdate(tileCache, navMesh) {
      let upToDate = false
      while (!upToDate) {
        const result = tileCache.update(navMesh)
        upToDate = result.upToDate
      }
    }

    for (var i = 0; i < 1000; i++) {
      const obj = new THREE.Mesh(new THREE.BoxGeometry(5,5,5), new THREE.MeshBasicMaterial({ color: 0xFF1133, wireframe: true }))
      scene.add(obj)

      const x = (i % 10) * 10
      const z = -200 + Math.floor(i / 10) * 10
      const y = 0

      obj.position.set(x, y, z)
      obj.geometry.computeBoundingBox()
      const bbox = obj.geometry.boundingBox
      const extents = bbox.max.clone().sub(bbox.min).divideScalar(2)

      const result = tileCache.addBoxObstacle(
        obj.position,
        extents,
        obj.rotation.y,
      )
      if (!result.success) throw new Error('addBoxObstacle failed')

      // 20 seems to be the highest reliable interval
      if (i % 20 === 0) {
        tileCacheFullUpdate(tileCache, navMesh)
      }
    }
isaac-mason commented 2 months ago

Thanks for looking into this more!

btw the result of addBoxObstacle is always a success.

Noted! This may help narrow down the issue.

isaac-mason commented 2 months ago

I'll create a storybook example like yours as a testbed to investigate, and so we've got an example of correct usage when we get a handle on this.

isaac-mason commented 2 months ago

I've added a storybook now, see: https://github.com/isaac-mason/recast-navigation-js/commit/56f1855161bfb56c6857f0dbbfe6b1bf8ba8f637 https://recast-navigation-js.isaacmason.com/?path=/story/tilecache-obstacles--many-obstacles-example

I think it's better to check the returned status, the tile cache queue size isn't exposed anywhere and could change. If the status is BUFFER_TOO_SMALL I do a full tile cache update then retry adding the obstacle.

let obstacles = 0;
for (let x = -10; x < 10; x += 2) {
  for (let z = -10; z < 10; z += 2) {
    obstacles += 1;

    const obstaclePosition = new Vector3(x, 0, z);

    const createObstacle = () => {
      if (obstacles % 2 === 0) {
        return tileCache.addBoxObstacle(
          obstaclePosition,
          boxObstacleSize,
          0.2
        );
      } else {
        return tileCache.addCylinderObstacle(
          obstaclePosition,
          addCylinderObstacleRadius,
          1
        );
      }
    };

    const result = createObstacle();

    if (
      !result.success &&
      statusDetail(result.status, Raw.Detour.BUFFER_TOO_SMALL)
    ) {
      fullTileCacheUpdate();

      createObstacle();
    }
  }
}
isaac-mason commented 2 months ago

I was also able to reproduce the issue with tile cache calls returning a successful status but not updating the navmesh as expected. In that storybook, if I change cs from 0.1 to 0.05, all tile cache calls (addBoxObstacle, addCylinderObstacle, update) are successful but many of the obstacles aren't reflected on the navmesh.

DVLP commented 1 month ago

This may be something worth raising with the main library https://github.com/recastnavigation/recastnavigation

isaac-mason commented 1 month ago

Yep agreed. Ideally we'd also create a reproduction with the c++ library directly to confirm it's not just an issue with the javascript bindings.