reeseschultz / ReeseUnityDemos

Unity packages and demos—emphasizing ECS, jobs and the Burst compiler—by Reese and others.
https://reese.codes
MIT License
516 stars 45 forks source link

Some area of NavPerformanceDemo is not reachable #24

Closed kobechenyang closed 4 years ago

kobechenyang commented 4 years ago

Hi ! Great work of the DOTS Navigation system. Great performance. I noticed a small problem with NavPerformanceDemo with unity 2019.3.15f1 that some area of the baked mesh surface is not reachable. See the attached sreenshot. WX20200604-171935 I dig into the source code a bit. Found out it was physics raycast failed

if (!physicsWorld.CastCollider(castInput, out ColliderCastHit hit))
{
      commandBuffer.RemoveComponent<NavNeedsDestination>(entityInQueryIndex, entity); // Ignore invalid destinations.
       return;
}

I have to increase the radius of sphere collider here from 1 to larger number to make it work

var collider = SphereCollider.Create(
new SphereGeometry()
{
     Center = destination.Value,
     Radius = 1
},

I guess it is unity physics bug, just want to bring it up incase someone ran into the same problem as me.

reeseschultz commented 4 years ago

Hey @kobechenyang, thank you so much for looking into this—you found a problem and you fixed it—I appreciate that. Parameterization is always a challenge.

Part of the issue appears to be that the agent.Offset probably shouldn't used to set the destinations for the NavPerformanceDemo:

commandBuffer.AddComponent(entityInQueryIndex, entity, new NavNeedsDestination{
  Value = NavUtil.GetRandomPointInBounds(
    ref random,
    renderBoundsFromEntity[surface.Value].Value,
    agent.Offset, // This shouldn't need to be factored into setting a dest, since it's used later.
    99
  )
});

That offset, if (0, 1, 0) which is what I initialize it to, is then that much farther from the surface to raycast. So if the radius of the sphere collider is one, that puts the boundary of the raycast right at the edge of the surface.... Whoops, lol.

Additionally, what's weird about this is that there may be a hair-thin difference with rounding on the positive z side vs the negative, but that's pure conjecture on my part. I'll make a mental note to investigate that separately.

Anyway, I'll release a patch for this later today. Thanks again, and please let me know if you need anything.

Edit: Actually, another issue is that the radius is hard-coded. That's not right. I'll add a constant to replace it, and increase the size.

reeseschultz commented 4 years ago

I resolved this issue with these commits: 53ea8eb6cb68caa3c6c14fba3391aaf8cef0bf26, cbe171d41f5468a730856ec9a4119036a593f92b, 669d7ea221bd7e9619f55cc2b5196714d6292b52.

Turns out no adjustment to the radius is needed. The main problem is that the performance and jump demos are adding agent.Offset into the initial destination float3, but that's just me using my own library wrong. The destination should be as close to the associated surface as possible, but I introduced the DESTINATION_SURFACE_COLLIDER_RADIUS constant just in case someone wants to update it from NavConstants.

Patch will be out shortly.

kobechenyang commented 4 years ago

ah I see. Thanks for the proper fix. It is weird that it is only not working on one side of the the surface. I am new to ECS. Your demo really helps me a lot. Thanks !