[PATCH v2] dsound: Correctly calculate angle between vectors with equal and opposite directions
Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> --- v2: Remove old calculation, thanks Nikolay. dlls/dsound/sound3d.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dlls/dsound/sound3d.c b/dlls/dsound/sound3d.c index 03fd3d4678..6ca79c1bbc 100644 --- a/dlls/dsound/sound3d.c +++ b/dlls/dsound/sound3d.c @@ -111,7 +111,13 @@ static inline D3DVALUE AngleBetweenVectorsRad (const D3DVECTOR *a, const D3DVECT return 0; cos = product/(la*lb); - angle = acos(cos); + if(fabs(1 - cos) < 0.0001f){ + angle = 0; + }else if(fabs(-1 - cos) < 0.0001f){ + angle = M_PI; + }else{ + angle = acos(cos); + } TRACE("angle between (%f,%f,%f) and (%f,%f,%f) = %f radians (%f degrees)\n", a->x, a->y, a->z, b->x, b->y, b->z, angle, RadToDeg(angle)); return angle; -- 2.18.0
Hi, Just curious, which cases does this fix? The ones when fabs(cos)>1 due to rounding errors? Or is acos broken for arguments near but not exceeding ±1? Regards, Ruslan On 23/07/2018, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> ---
v2: Remove old calculation, thanks Nikolay.
dlls/dsound/sound3d.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/dlls/dsound/sound3d.c b/dlls/dsound/sound3d.c index 03fd3d4678..6ca79c1bbc 100644 --- a/dlls/dsound/sound3d.c +++ b/dlls/dsound/sound3d.c @@ -111,7 +111,13 @@ static inline D3DVALUE AngleBetweenVectorsRad (const D3DVECTOR *a, const D3DVECT return 0;
cos = product/(la*lb); - angle = acos(cos); + if(fabs(1 - cos) < 0.0001f){ + angle = 0; + }else if(fabs(-1 - cos) < 0.0001f){ + angle = M_PI; + }else{ + angle = acos(cos); + } TRACE("angle between (%f,%f,%f) and (%f,%f,%f) = %f radians (%f degrees)\n", a->x, a->y, a->z, b->x, b->y, b->z, angle, RadToDeg(angle)); return angle; -- 2.18.0
On Mon, Jul 23, 2018 at 10:51:49PM +0300, Ruslan Kabatsayev wrote:
Just curious, which cases does this fix? The ones when fabs(cos)>1 due to rounding errors? Or is acos broken for arguments near but not exceeding ±1?
Pretty sure it's rounding errors that result in fabs(cos)>1. That causes acos to return NAN, and bad things happen to audio volumes. I'd be surprised if acos is broken like the 2nd case you mention. Andrew
On 23/07/2018, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> ---
v2: Remove old calculation, thanks Nikolay.
dlls/dsound/sound3d.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/dlls/dsound/sound3d.c b/dlls/dsound/sound3d.c index 03fd3d4678..6ca79c1bbc 100644 --- a/dlls/dsound/sound3d.c +++ b/dlls/dsound/sound3d.c @@ -111,7 +111,13 @@ static inline D3DVALUE AngleBetweenVectorsRad (const D3DVECTOR *a, const D3DVECT return 0;
cos = product/(la*lb); - angle = acos(cos); + if(fabs(1 - cos) < 0.0001f){ + angle = 0; + }else if(fabs(-1 - cos) < 0.0001f){ + angle = M_PI; + }else{ + angle = acos(cos); + } TRACE("angle between (%f,%f,%f) and (%f,%f,%f) = %f radians (%f degrees)\n", a->x, a->y, a->z, b->x, b->y, b->z, angle, RadToDeg(angle)); return angle; -- 2.18.0
Hi, On 24/07/2018, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
On Mon, Jul 23, 2018 at 10:51:49PM +0300, Ruslan Kabatsayev wrote:
Just curious, which cases does this fix? The ones when fabs(cos)>1 due to rounding errors? Or is acos broken for arguments near but not exceeding ±1?
Pretty sure it's rounding errors that result in fabs(cos)>1. That causes acos to return NAN, and bad things happen to audio volumes. I'd be surprised if acos is broken like the 2nd case you mention.
Then why not check for the condition explicitly instead of introducing an arbitrary "tolerance" constant and working in symmetric neighborhoods of ±1? I.e. I suggest using `cos>1` condition in the first 'if' and `cos<-1` in the second one. IMHO it'd be easier to understand for a casual reader. (This will change behavior in the cases of |cos|≫1, but we aren't trying to guard against them, are we?) Regards, Ruslan
Andrew
On 23/07/2018, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
Signed-off-by: Andrew Eikum <aeikum(a)codeweavers.com> ---
v2: Remove old calculation, thanks Nikolay.
dlls/dsound/sound3d.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/dlls/dsound/sound3d.c b/dlls/dsound/sound3d.c index 03fd3d4678..6ca79c1bbc 100644 --- a/dlls/dsound/sound3d.c +++ b/dlls/dsound/sound3d.c @@ -111,7 +111,13 @@ static inline D3DVALUE AngleBetweenVectorsRad (const D3DVECTOR *a, const D3DVECT return 0;
cos = product/(la*lb); - angle = acos(cos); + if(fabs(1 - cos) < 0.0001f){ + angle = 0; + }else if(fabs(-1 - cos) < 0.0001f){ + angle = M_PI; + }else{ + angle = acos(cos); + } TRACE("angle between (%f,%f,%f) and (%f,%f,%f) = %f radians (%f degrees)\n", a->x, a->y, a->z, b->x, b->y, b->z, angle, RadToDeg(angle)); return angle; -- 2.18.0
On Tue, Jul 24, 2018 at 04:51:21PM +0300, Ruslan Kabatsayev wrote:
Hi,
On 24/07/2018, Andrew Eikum <aeikum(a)codeweavers.com> wrote:
On Mon, Jul 23, 2018 at 10:51:49PM +0300, Ruslan Kabatsayev wrote:
Just curious, which cases does this fix? The ones when fabs(cos)>1 due to rounding errors? Or is acos broken for arguments near but not exceeding ±1?
Pretty sure it's rounding errors that result in fabs(cos)>1. That causes acos to return NAN, and bad things happen to audio volumes. I'd be surprised if acos is broken like the 2nd case you mention.
Then why not check for the condition explicitly instead of introducing an arbitrary "tolerance" constant and working in symmetric neighborhoods of ±1? I.e. I suggest using `cos>1` condition in the first 'if' and `cos<-1` in the second one. IMHO it'd be easier to understand for a casual reader. (This will change behavior in the cases of |cos|≫1, but we aren't trying to guard against them, are we?)
Yeah, that makes sense, thanks for the suggestion. I'll re-send. Andrew
participants (2)
-
Andrew Eikum -
Ruslan Kabatsayev