Posts
__forceinline float tan(float a)
{
return ::tanf(clamp(a, (-1.0f + FUZZY_EPSILON), (1.0f - FUZZY_EPSILON)));
}
__forceinline double tan(double a)
{
return ::tan(clamp(a, (-1.0 + FUZZY_EPSILON), (1.0 - FUZZY_EPSILON)));
}
We've had this in our code base since 2005 and have shipped 3 games on it, solid!
7 comments:
I have two non-facetious questions about this:
1) why was it done? Seems to have been implemented with reasonable care, is there any indication in any record anywhere why?
2) why is it a bug? If three products have been shipped on this code in five years then...what's the problem?
I can guess. the tan function diverges at pi/2 and -pi/2, so the original authors were probably trying to "cut out" the singularities (but if that's true they picked a pretty narrow range, since tan(1) is only around 1.5).
It's a bug because that's not how the tangent function is defined. For example, it should be identically true that tan(2pi+pi/4) = tan(pi/4). Even if no one tripped over this in 5 years, it's still a bug waiting to happen.
Don't get me wrong, there's a lot to raise concerns about here in a code review. At the very least, these functions have very misleading names as indeed they do not implement a tangent function.
What I'm more interested in, though, is the programmer psychology that would assume it will be obvious to all comers that this code is defective and post it to a blog like this one. Particularly since the latent defect here seems, on the evidence presented, to be very latent indeed.
The psychological aspect here and reason for posting such code snippets is that most everyone that reads this blog have been burned before by exactly these types of bugs & issues, and as such can relate to them and the hardship and/or fun times they often have presented.
It might not be for everyone, esp. not with the hardcore obtuse & implicit references that is required to understand the code snippets or images. But that is fine and precisely one of the things the Internets are good for (connecting tiny niche communities)
And lastly, seeing stuff that have failed can simply just be genuinely fun! "All aboard the fail boat"
@repi
Sure. I understand what this blog is about. I want to get at something more specific with this example.
I don't like the term "bug". I prefer the model (I think due to Meyer) that programmers make errors and as a result inject defects into their code which might or might not manifest as failures in production.
So, I ask you: what failures in production has this code caused?
And, if you believe it to be defective (I suppose so, since you posted it), what specific error do you think the programmer who wrote it made?
We don't know yet if it has caused any failures in shipping products or failures during development that have cost extra time to work around as this defect hadn't been found in so many years.
Most likely this error can create soft errors in areas of game code that typically are hard to find & debug, esp. as the user of the function often wouldn't consider that the core tan function is incorrectly implemented.
We have source revision history back to the exact changelist that added this defect and the intent of the programmer was to clamp unsafe inputs of multiple trigonometric functions that can require it such as atan & acos but at the same time incorrectly added the clamping to this function as well in the same change.
Easy mistake to do when copy'n'pasting code between the trigonometric functions without fully checking what the difference between them is.
@repi
I see. Many thanks for the additional detail.
Knowing that there is evidence from the version control that the programmer did make an error when this was written makes it much easier for me to interpret as a defect (rather than as a sloppily named implementation of an obscure thing).
Post a Comment