PolyToCol applied in RecastDemo #254

domalb · 2017-01-30T23:40:33Z

While the code is hot, I implemented a specific polyToCol in RecastDemo to make it illustrated in the library codebase itself.

Main changes :

created sampleAreaToCol which computes a color from the specific demo area flags. Chosen color constants may be reviewed to fit correctly with the rest of the debug drawn elements.
created SampleDebugDraw which inherits from DebugDrawGL and implements polyToCol by simply calling sampleAreaToCol
added SampleDebugDraw m_dd as attribute of Sample and used it anywhere a DebugDrawGL was created on stack
- first because I felt it better factored to choose the type and declare it once instead of in every draw function
- then because those draw function do not need to know the concrete type of dd, duDebugDraw should be enough
- finally because it could be useful (in the future) to have access to a long lasting instance and set some attributes (like draw options or area colors...)
used sampleAreaToCol in drawing of convex volumes so that their color match the area they set
updated the imguiCheck for convex volumes so that all SamplePolyAreas values are covered, except GROUND, which corresponds to zero, i.e. the default area value. Maybe adding GROUND could be useful too. To be discussed.

Note
I noticed that TileCachePolyMesh and TileCacheLayerAreas (maybe others I missed) are also drawn with a color determined from area. I'm wondering whether changing the custom color from polyToCol(const dtPoly* poly) to areaToCol(unsigned int area) would be a good idea. On one hand it could apply to more elements (at least TileCachePolyMesh, TileCacheLayerAreas and sample convex volumes) with no user extra code, but on the other hand it gives some limitations in the case of polygons (restriction to areas, not flags or any other info).
What do you think about that ?

Implement custom functions to be called when an assertion fails.

jakobbotsch · 2017-01-31T17:12:40Z

RecastDemo/Source/ConvexVolumeTool.cpp

-	if (imguiCheck("Road", m_areaType == SAMPLE_POLYAREA_ROAD))
-		m_areaType = SAMPLE_POLYAREA_ROAD;
+// 	if (imguiCheck("Ground", m_areaType == SAMPLE_POLYAREA_GROUND))
+// 		m_areaType = SAMPLE_POLYAREA_GROUND;


Why are these commented?

As noted above

updated the imguiCheck for convex volumes so that all SamplePolyAreas values are covered, except GROUND, which corresponds to zero, i.e. the default area value. Maybe adding GROUND could be useful too. To be discussed.

More precisely, the SAMPLE_POLYAREA_GROUND area flag value is zero. Creating a convex volume with SAMPLE_POLYAREA_GROUND area actually makes holes in the built mesh, which is not coherent with the label "Ground".

The fact that default walkable area end with SAMPLE_POLYAREA_GROUND can be considered as a misleading trick in the sample. Default walkable area is defined by RC_WALKABLE_AREA with value 63, whereas RC_NULL_AREA equals 0 and is considered as making holes in the mesh.
The trick allows to reuse the zero value in areas. It appears in the build process in specific sample code
if (m_pmesh->areas[i] == RC_WALKABLE_AREA) m_pmesh->areas[i] = SAMPLE_POLYAREA_GROUND; which is misleading in my opinion. It induces confusion between SAMPLE_POLYAREA_GROUND and RC_NULL_AREA.

I would rather give a new non-zero value to SAMPLE_POLYAREA_GROUND, and expose the zero-value area convex volume as "Null" or "Not walkable" or "Hole"

Yes you are right about the holes.

The reasoning for this is that area types change meaning in the build process.
Until regions are built there is a special flag to indicate holes - this flag is RC_NULL_AREA which as you point out coincides with SAMPLE_POLYAREA_GROUND.

After regions have been built this flag is no longer meaningful, and Recast can support the entire range of areas (64 different areas). The remapping is handled here.

The original source of the RC_WALKABLE_AREA flag is rcMarkWalkableTriangles.
This function is used in the sample.

So several options for you:

Leave it alone for now and uncomment it again. It can be fixed in a future PR.

Resolve it by remapping SAMPLE_POLYAREA_GROUND to RC_WALKABLE_AREA during convex poly marking

The ideal solution (which should happen separately from these changes) is something like:
RC_WALKABLE_AREA is not meaningful to the build process in any way, it is simply a flag with value 63. rcMarkWalkableTriangles should be changed to accept the flag to use for marking the walkable triangles instead of always using flag 63. After this change all the SAMPLE_POLYAREA values should have 1 added to them (or implicitly add 1 during rasterization and subtract 1 in the poly mesh), and SAMPLE_POLYAREA_GROUND should be passed to rcMarkWalkableTriangles.

I don't expect you to submit a PR for this last bit. If you don't I will probably do it some time in the future when I have time, but please do either 1 or 2.

Anyway thanks for the work!

Ok, done with 1.

jakobbotsch · 2017-01-31T17:13:05Z

I think the area change would be good to take asap, so if you could make a PR with that separately it would be good.

That said, I like how these changes make the colors meaningful. Can you please rebase your commits in addition to fixing my comment?

domalb · 2017-01-31T20:58:22Z

I must admit that I'm fairly new with Git, and struggling with GitHub client for Windows.
I didn't manage to fork recastnavigation/recastnavigation a second time as I already have one fork... Why is that limitation ?
I didn't manage either to work with branches, as all branches are switched in the same local folder and it is forbidden if some local changes have not been commited (which sounds unsafe regarding to auto-add to running PR).

Sorry for those lamentations, but I can't figure how to make a new separate PR : /
But I would be glad to !

jakobbotsch · 2017-01-31T21:23:46Z

I am not familiar with the GitHub client for Windows unfortunately. With the CLI it would look something like:

git clone git@github.com:domalb/recastnavigation.git
cd recastnavigation
# Add this repo as upstream repo to be able to sync
git remote add upstream git@github.com:recastnavigation/recastnavigation.git
git fetch upstream
# Create an area-to-col branch from your current branch (master)
git checkout -b area-to-col
# Since you have made changes to master in your own fork, we need to sync this new branch to our upstream instead of your fork
git reset --hard upstream/master
# Make your changes here
git add .
git commit -m 'Area to col instead'
# Push this new branch to GitHub
git push -u origin area-to-col

After this you should be able to create a pull request from the GitHub home page.

One way to resolve your problem with your current master is something like this (I am sure there are better ways):

# Let's create a branch meaningful-colors to put the color changes to instead of master
git checkout -b meaningful-colors
# Sync up with upstream
git reset --hard upstream/master
# Get the two commits you already have from master
git cherry-pick a86c81d93..7e44bae
git push -u origin meaningful-colors

After that you should be able to create a new PR for this issue that comes from the new branch instead of your master. Then you will need to reset your master to be in sync with us:

git checkout master
git reset --hard upstream/master

The git reset --hard are only necessary since your master is differing from ours. Usually your work flow would look something like

git checkout master
git pull --rebase upstream/master
git checkout -b some-branch
# make changes
git push -u origin some-branch

grahamboree · 2017-01-31T23:38:19Z

An arguably easier way to rebase your changes (provided everything's up to date) is to rebase and force-push.

# Make sure we're on master
git checkout master

# Get up to date
git pull
git fetch upstream

# Rebase and force-push your master branch (since that's what this PR is from)
git rebase upstream/master
git push --force origin/master

That will automatically update this PR with your changes rebased. In the future it's probably easier to work in separate branches like @jackpoz suggests.

hymerman · 2017-02-01T09:47:05Z

Give Graham's suggestion a go (it's useful to get acquainted with the git command line :) ) - but if you struggle, it's possible for one of us to grab your fork, rebase and push it ourselves, keeping your name in the commits - let us know :)

jackpoz · 2017-02-01T09:52:07Z

In the future it's probably easier to work in separate branches like @jackpoz suggests.

very nice suggestion, but wrong ja* nick tagged :)

domalb · 2017-02-01T17:50:31Z

I merged the AreaToCol stuff

jakobbotsch · 2017-02-01T18:50:54Z

RecastDemo/Include/Sample.h

+class SampleDebugDraw : public DebugDrawGL
+{
+public:
+	virtual unsigned int polyToCol(const struct dtPoly* poly);


Has to be areaToCol now.

jakobbotsch · 2017-02-01T18:52:36Z

RecastDemo/Source/Sample.cpp

+	}
+}
+
+unsigned int SampleDebugDraw::polyToCol(const struct dtPoly* poly)


Also needs update

Of course !
My bad, too much used to rely on the override keyword.
Should be ok now.

jakobbotsch · 2017-02-02T19:28:14Z

Thanks!

jakobbotsch · 2017-02-02T19:55:28Z

I have noticed that the default color has changed significantly. I am not sure if this was intentional, but for now I have changed it back in #256 to retain the old look (by default).

domalb added 10 commits January 14, 2017 00:12

Assertion failure custom functions

a5902e2

Implement custom functions to be called when an assertion fails.

Assertion functions globals not defined if NDEBUG

a32f36d

Replaced NULL with 0

ca92779

Debug draw PolyToCol custom function

ab93dda

Merge remote-tracking branch 'refs/remotes/recastnavigation/master'

3fd36cc

PolyToCol as duDebgDraw virtual method

19b3014

duDebugDraw polyToCol without alpha argument

6241c9a

Merge remote-tracking branch 'refs/remotes/recastnavigation/master'

3628014

Apply polyToCol to RecastDemo

a86c81d

Remove unexpected demo bin file

7e44bae

jakobbotsch requested changes Jan 31, 2017

View reviewed changes

domalb added 2 commits January 31, 2017 22:24

uncommented ground convex volume

70721ef

uncommented ground convex volume (erratum)

f08af58

Merge remote-tracking branch 'refs/remotes/recastnavigation/master'

724cee0

jakobbotsch requested changes Feb 1, 2017

View reviewed changes

Fixed AreaToCol in sample

1f4ac02

jakobbotsch approved these changes Feb 2, 2017

View reviewed changes

jakobbotsch merged commit 03eb2f9 into recastnavigation:master Feb 2, 2017

jakobbotsch mentioned this pull request Feb 2, 2017

Change back to previous default color #256

Merged

jakobbotsch mentioned this pull request Feb 5, 2017

Camera moves up/down and arrows #259

Merged

PolyToCol applied in RecastDemo #254

PolyToCol applied in RecastDemo #254

Uh oh!

Conversation

domalb commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

domalb Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

domalb Jan 31, 2017

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Jan 31, 2017

Uh oh!

domalb commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jakobbotsch commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

grahamboree commented Jan 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hymerman commented Feb 1, 2017

Uh oh!

jackpoz commented Feb 1, 2017

Uh oh!

domalb commented Feb 1, 2017

Uh oh!

jakobbotsch Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

domalb Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

jakobbotsch commented Feb 2, 2017

Uh oh!

jakobbotsch commented Feb 2, 2017

Uh oh!

Uh oh!

domalb commented Jan 30, 2017 •

edited

Loading

domalb Jan 31, 2017 •

edited

Loading

domalb commented Jan 31, 2017 •

edited

Loading

jakobbotsch commented Jan 31, 2017 •

edited

Loading

grahamboree commented Jan 31, 2017 •

edited

Loading