Closed briansemrau closed 1 year ago
In the case where the polygon is defined by the programmer, an assert is desirable. With your proposed changes, that would require changing this
shape.Set(vertices, count);
to this
[[maybe_unused]] const bool valid = shape.Set(vertices, count);
b2Assert(valid);
to maintain the same behaviour. I think the current Set
function should be kept as it as with a separate function for validation.
I'd generally argue against using asserts in functions with nontrivial failure cases. There's no reason to assume that every polygon is going to be made using predefined data. However, I can also see many reasons why it is done that way, so I agree that adding a function to pre-validate a set of points would be the best solution to this.
I will add a function to check for validity.
Heyhey, just stumbled across this issue. Looking at the b2PolygonShape::Set code I'm kinda curious how this is going to work out because Set
basically runs 2 internal algorithms that can lead to function abort: unique-folding and this Gift Wrap thingy.
So the polygon validator would have to run those 2 as well and then you can call Set with your validated poly which runs them again? (because it doesn't/can't rely on only getting valid polys)
When
b2PolygonShape::Set
fails, whether it be because N<3, the polygon is degenerate, or otherwise, the function currently fails an assertion and callsSetAsBox
. This makes it impossible to check if we've passed a bad polygon to the function, which is crucial in projects where polygons can be fully defined by the user.Right now the only way around this is to copy the logic from within
b2PolygonShape::Set
to check the validity of our input before actually callingb2PolygonShape::Set
.I propose that instead of calling
b2Assert
and failing,b2PolygonShape::Set
should simply returnfalse
(and continue to callSetAsBox
as currently implemented - that part is good)