[PATCH] [ATLAS] Assertion failure in CBrush::Slice while editing map

[michael]

Assertion failed: "idx < si.newv.size()" Location: Brush.cpp:246 (CBrush::Slice)

Call stack:

(error while dumping stack: No stack frames found) errno = 0 (No error reported here) OS error = 0 (no error code was set)

[Erik Johansson]

Can confirm this. After painting a bit I moved the camera around a bit and after rotating the camera a while I got the same error. More info:

Assertion failed: "idx < si.newv.size()"
Location: Brush.cpp:246 (CBrush::Slice)

Call stack:

00FA0A04

00FA0BFA

00F83697

00FCDC0D

01064CB1

00EE1119

00EE1A16

00FCDF4E

00FA9B4B

00FA9F15

01064CE6

00F667A4

00F667FB

00EFF913


errno = 0 (No error reported here)
OS error = 0 (no error code was set)

After I clicked Continue on that first error I got this:

Assertion failed: "si.newv[idx].neighb2 == prev"
Location: Brush.cpp:255 (CBrush::Slice)

Call stack:

00FA0A49

00FA0BFA

00F83697

00FCDC0D

01064CB1

00EE1119

00EE1A16

00FCDF4E

00FA9B4B

00FA9F15

01064CE6

00F667A4

00F667FB

00EFF913


errno = 13 (Insufficient access rights to open file)
OS error = 0 (no error code was set)

Not sure that's too relevant as it may be a consequence of the first one, but for completeness sake. And two more: ​http://pastebin.com/T8vq0tjE I'll attach the crashdump etc.

[historic_bruno]

Fast way to reproduce this:

• Create blank map
• Paint a texture
• Rotate the camera quickly with Ctrl+MiddleMouseButton+Drag, so that the camera goes under the terrain and back
• Crash

[historic_bruno]

#896 was the original report of this bug, so it has been a problem for some time and the affected code has not been changed since 2006. I'm not sure why it would cause more of a problem now, except that it's in some way connected to the camera frustum and maybe certain maps are more prone to the error in normal use.

One possible work around would be to disable shadows before editing your map, since this bug occurs when calculating the shadow map's bounds.

[Kieran P]

Bug still exists, but can be suppressed so the user can keep going.

[vts]

Currently looking into this, gaining an understanding of the code and documenting it better.

[historic_bruno]

Replying to vts:

and documenting it better

Excellent! :D

[vts]

This seems to be a result of the ShadowBound variable in renderer/ShadowMap.cpp being an empty bound. In ShadowMapInternals::CalcShadowMatrices, this bound is intersected by the lightspace camera viewing frustum; to do this, a CBrush is first created from the bound, and this brush is then sliced by each of the camera viewing frustum planes in turn.

When a brush is sliced by a plane, the distances of each vertex in the brush to the slicing plane are calculated, to determine which ones are cut off (distance < 0) and which ones are kept (distance >= 0). However, because the bound is empty, its min/max point coordinates are all at +/-FLT_MAX. Therefore, depending on the exact numeric values of the frustum planes, this can cause the distance calculations to overflow, leading to various nonsense results, which I believe ultimately leads up to this crash.

Interestingly, the ShadowBound is no longer empty as soon as you place a single building in Atlas, suggesting that somewhere there's an IsEmpty() check missing on the shadow bounds. Looking into it now.

[vts]

Attaching a patch for this issue. Does the following:

• Prevent this error from occuring by requiring a non-empty bound for CBoundingBoxAligned::IntersectFrustumConservative to run, and having ShadowMap::CalcShadowMatrices default to a no-op if the shadowbounds are empty.
• Better documentation of the CBrush class and its operations
• Adds a demonstrative testcase for CBrush::Slice

[vts]

Updated; forgot to change last-modified dates on files

[Philip Taylor]

• "if this bound is empty, it cannot be meaningfully intersected with a frustum" - wouldn't it be perfectly meaningful that the intersection of an empty bound and a non-empty shape is another empty bound?
• Multi-line "///" comments look ugly to me - probably better to use "/** .. */" in those cases.
• Unnecessary indentation in ShadowMapInternals::CalcShadowMatrices is ugly - better to do a

  if (shadowBound.IsEmpty())
  {
    do the identity thing;
    return;
  }
  

  at the top of the function, so the rest of the body doesn't need its indentation increased. Also it makes the code easier to read by keeping the condition and associated action near each other, rather than spreading from the start to the end of the function.

[vts]

• "if this bound is empty, it cannot be meaningfully intersected with a frustum" - wouldn't it be perfectly meaningful that the intersection of an empty bound and a non-empty shape is another empty bound?

I suppose that makes sense in a way similar to 0/x = 0. In most cases, the overflows inside the calculations actually ended up producing an empty resulting bound. Hence, I suppose defining the resulting bound to be empty if the starting bound is empty would make for a cleaner fix, because it wouldn't require any changes on the calling side.

At the time I wrote this patch, I preferred to explicitly disallow a intersection on an empty bound to force caller awareness of the latent nature of this bug, but I suppose defining the result to be empty would work just as well.

• Multi-line "///" comments look ugly to me - probably better to use "/** .. */" in those cases.

I typically tend to prefer // comments over /* */ comments because the latter preclude commenting out large sections of code at once, which can sometimes be useful when making large code changes. I'll change it if you like.

• Unnecessary indentation in ShadowMapInternals::CalcShadowMatrices is ugly - better to do a

  if (shadowBound.IsEmpty())
  {
    do the identity thing;
    return;
  }
  

  at the top of the function, so the rest of the body doesn't need its indentation increased. Also it makes the code easier to read by keeping the condition and associated action near each other, rather than spreading from the start to the end of the function.

Agreed; the current form is a remnant of an older version of this patch.

[vts]

Updated version of patch per Philip's comments

[vts]

In 11441:

Prevent AABB/frustum intersections from blowing up on empty input bounds. Fixes #1027.