search

Robmaister / SharpNav

Advanced Pathfinding for C#
sharpnav.com
Other
534 stars 165 forks source link

Bugfix - different from original c++ code #63

Open JYKeith123 opened 8 years ago

JYKeith123 commented 8 years ago

thanks for the great porting!

Robmaister commented 8 years ago

For the change in Heightfield.cs, why change Math.Ceiling to Math.Round? Wouldn't it make sense to always round up to ensure no geometry is missed? The values should always be positive, so there's no need to worry about rounding to or away from zero either...

As for the second change, nice catch! I'll wait to figure out what's going on with the Heightfield change before pulling this in.

JYKeith123 commented 8 years ago

Actually, I'm not sure for the Heightfield.cs change. But it seems to cause unit test failure in my case.

Currently, I'm making rts style game which needs deterministic programming. So I changed all float value to this fixed-point data type. Then ran all tests. There were a few failures, and found several code differences I committed into this repo between original C++ code and SharpNav code. After fixing it, unit tests were all passed.

The original code below. void rcCalcGridSize(const float* bmin, const float* bmax, float cs, int* w, int* h) { *w = (int)((bmax[0] - bmin[0])/cs+0.5f); *h = (int)((bmax[2] - bmin[2])/cs+0.5f); }

Robmaister commented 8 years ago

interesting, do you have separate unit tests for SharpNav? If so I'd be happy to pull in more for the SharpNavTests project.

Looking at Recast's tests, this would be an incompatibility between the two, but the project is not supposed to be a straight port - a lot of things are moved around structurally, and I generally prefer correctness over slight memory savings.

The only difference is that SharpNav should have an extra column and row that contains the last 0.5m of the world that Recast does not. I would rather not pull in that change unless there's a specific issue caused by having those extra cells. Does that make sense?

JYKeith123 commented 8 years ago

No, All unit tests I did are from SharpNavTests project. I just changed float type to fixed-point type.

Okay. I agree with you since there is no specific reason. I'll let you know if I catch other issues about this. Cheers :-)

Robmaister commented 8 years ago

I'll do some testing and see why ceiling isn't working for you but round does. My guess is it has something to do with round using banker's rounding (in the case of 0.5, round to the nearest even), still not sure why ceiling wouldn't work for you though.

If you have a copy of the projects set up with the Fix64 type, could you send that my way to speed up the process? I'm fine with a zipped folder over email or whatever if you don't have it up on a public repo somewhere.

JYKeith123 commented 8 years ago

Hi, Here is the link.

I noticed that I converted SharpNav into .Net 3.5 as well, to share with unity project. (My project will be used for both unity client and .net server) And also changed NUnit to MSTest for integrating with my other tests. Sorry for not mentioning this.

If you run all tests from the solution, you can see this 3 test failures while ceiling the values.

BuildRegions_Success, DistanceField_Medium_Success, DistanceField_Simple_Success

And you change Heightfield.cs ceiling to round, then all tests will succeed.

Please see files & folders explanation below. Files

Folders

Robmaister commented 8 years ago

Thanks, I'll take a peek at this probably early next week