[PATCH]shortPathfinder may return paths in non passable region.

[mimo]

There is a defect in the short pathfinder when dealing with obstructions. It can enter inside the obstruction and then continue its path in non passable regions. This is easily reproducile with a dock. Putting a unit along the dock, at the limit of the water and asking it to go just a few meters (smaller than the short/long separation) inside the water. The short pathfinder will make it go round the dock. Same thing with a ship and ask it to go a few meters in the land. This is one of the reasons for bugs as #3472 because as soon as there is a dock around, ship movements become completely crazy due to this problem. The patch attached is a (hacky) fix.

[mimo]

In 17765:

prevent short pathfinder to go into impassable region, fixes #3785

[mimo]

Although i've not noticed problems with r17765, it could be too restrictive is some cases to forbid paths with an impassable vertex. The new patch in r17768 allows back such vertices, but consider all paths between two impassable vertices as non visible.

[wraitii]

Reopening, Mimo's fix is most likely not entirely accurate. A building may have all its vertexes inside it's impassable raster, so it would be illegally discarded by the above fix, which may lead to weird paths now and then. I don't believe this is very likely, but it could cause trouble. In general, it also lead to some over computation of passability.

The below patch check only whether the target vertex is on impassable terrain, and discards it if so.

[gameboy]

This patch can completely solve the problem?

[gameboy]

My friend, if you have forgotten this problem？

[wraitii]

Simply expand the area we'll compute the terrain obstructions in. Fixes the issue more neatly, as used in #4327.

[elexis]

Moving to the new WIP milestone.

[wraitii]

In 19024:

Better fix to #3785. Fixes #3785

[Stan]

This looks like a little duplicated code, why can't a for : be used here ?

[elexis]

With gcc 5.4.1:

../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp: In member function ‘virtual void CCmpPathfinder::ComputeShortPath(const IObstructionTestFilter&, entity_pos_t, entity_pos_t, entity_pos_t, entity_pos_t, const PathGoal&, pass_class_t, WaypointPath&)’:
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:618:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:618:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:619:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:619:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:621:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:621:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:622:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:622:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:624:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:624:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:625:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:625:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:627:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:627:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X < rangeXMin) rangeXMin = vert.p.X; if (vert.p.Y < rangeZMin) rangeZMin = vert.p.Y;
                                                   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:628:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:628:51: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (vert.p.X > rangeXMax) rangeXMax = vert.p.X; if (vert.p.Y > rangeZMax) rangeZMax = vert.p.Y;
                                                   ^~

Agreeing with the compiler that every statement should be on an individual line. I subjectively prefer the x = std::max(x, y) pattern, but be careful in case you rearrange (it also supports the initializer list std::max({x, y, z}) though you can't use that here by the looks of things).

---

Re Stan: Removing code duplication in that hunk doesn't seem effective due to the slight differences in those lines and cpp being less flexible than JS.

[wraitii]

Grmbl, compilers can be really annoying now and then. might use the std::max trick, will see what's more readable.

For stan's comment, what you said + this part of the code is kind of never looked at and already repetitive and also doomed to be replaced one day so who cares.

[Stan]

Okay makes sense. Just asking cause sometimes cleanup is good to have.

[wraitii]

In 19041:

Fix misleading indentation warning on gcc, introduced by [19024]. Reported by elexis. Fixes #3785

[elexis]

In r19043 by wraitii:

Fix a bug introduced by a fix of a bug introduced by a fix of a bug (x100) in the vertex pathfinder. Revealed by automated tests on another branch of mine. Revert back to pre-new pathfinder behavior of not returning paths in some cases, since that is most likely less broken than now - but probably still a little broken.

[elexis]

With gcc 5.4.1:

../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp: In member function ‘virtual void CCmpPathfinder::ComputeShortPath(const IObstructionTestFilter&, entity_pos_t, entity_pos_t, entity_pos_t, entity_pos_t, const PathGoal&, pass_class_t, WaypointPath&)’:
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:619:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (aa) vert.quadInward = QUADRANT_BR; vertexes.push_back(vert);
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:619:42: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (aa) vert.quadInward = QUADRANT_BR; vertexes.push_back(vert);
                                          ^~~~~~~~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:625:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (aa) vert.quadInward = QUADRANT_TR; vertexes.push_back(vert);
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:625:42: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (aa) vert.quadInward = QUADRANT_TR; vertexes.push_back(vert);
                                          ^~~~~~~~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:631:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (aa) vert.quadInward = QUADRANT_TL; vertexes.push_back(vert);
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:631:42: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (aa) vert.quadInward = QUADRANT_TL; vertexes.push_back(vert);
                                          ^~~~~~~~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:637:3: warning: this ‘if’ clause does not guard... [-Wmisleading-indentation]
   if (aa) vert.quadInward = QUADRANT_BL; vertexes.push_back(vert);
   ^~
../../../source/simulation2/components/CCmpPathfinder_Vertex.cpp:637:42: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the ‘if’
   if (aa) vert.quadInward = QUADRANT_BL; vertexes.push_back(vert);
                                          ^~~~~~~~

[mimo]

I'm even not sure r19043 was ever tested on game as 0ad is now unplayable with lots of units being blocked. I don't even know why it was needed to modify r17768, but this succession of untested and unreviewed patches is not what we should expect.

[fatherbushido]

I have indeed stuff like stucked units, some finally find a path, some doesn't.

[elexis]

In r19054 by wraitii:

Revert [19043]

[elexis]

Backlogging due to lack of progress

[Imarok]

Move tickets to Simulation as UI & Simulation got some sub components.