Bug 34548 – audio engine: add Vector3 class

• Component: Platform
• Status: RESOLVED FIXED

[Chris Rogers]

This is a class for a 3D vector, implementing such methods as dot and cross product.
It's needed for 3D audio spatialization.

[Chris Rogers]

Created attachment 48082 [details]
patch for 3D vector class

[WebKit Review Bot]

Attachment 48082 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/platform/audio/VectorTypes.h:90:  Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons.  [readability/comparison_to_zero] [5]
WebCore/platform/audio/VectorTypes.h:95:  Mismatching spaces inside () in if  [whitespace/parens] [5]
WebCore/platform/audio/VectorTypes.h:165:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 3


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Rogers]

Created attachment 48084 [details]
fix minor style violations

[Mark Rowe (bdash)]

The convention used in WebKit is to have one class per .h file, and to name the .h file after the class it contains.  This makes it more obvious where to find the class.  In this case it would have the advantage of making it much less likely to confuse the file with something related to WTF::Vector.

[Chris Rogers]

Created attachment 48086 [details]
change filename from VectorTypes.h to Vector3.h

Hi Mark, thanks for looking at this.  I've changed the filename to Vector3.h to match the class-name...

[Sam Weinig]

Comment on attachment 48086 [details]
change filename from VectorTypes.h to Vector3.h

> Index: WebCore/ChangeLog
> ===================================================================
> --- WebCore/ChangeLog	(revision 54315)
> +++ WebCore/ChangeLog	(working copy)
> @@ -1,3 +1,30 @@
> [phone]  Chris Rogers  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        audio engine: add Vector3 class
> +        https://bugs.webkit.org/show_bug.cgi?id=34548
> +
> +        No tests since no javascript API yet
> +
> +        * platform/audio: Added.
> +        * platform/audio/Vector3.h: Added.
> +        (WebCore::Vector3::Vector3):
> +        (WebCore::Vector3::abs):
> +        (WebCore::Vector3::isZero):
> +        (WebCore::Vector3::normalize):
> +        (WebCore::Vector3::x):
> +        (WebCore::Vector3::y):
> +        (WebCore::Vector3::z):
> +        (WebCore::operator+):
> +        (WebCore::operator-):
> +        (WebCore::operator*):
> +        (WebCore::dot):
> +        (WebCore::cross):
> +        (WebCore::Vector3::dot):
> +        (WebCore::Vector3::cross):
> +        (WebCore::Vector3::distance):
> +
>  2010-02-03  Adele Peterson  <[e-mail]>
>  
>          Reviewed by Brady Eidson.
> Index: WebCore/platform/audio/Vector3.h
> ===================================================================
> --- WebCore/platform/audio/Vector3.h	(revision 0)
> +++ WebCore/platform/audio/Vector3.h	(revision 0)
> @@ -0,0 +1,165 @@
> +/*
> + * Copyright (C) 2010 Google Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1.  Redistributions of source code must retain the above copyright
> + *     notice, this list of conditions and the following disclaimer.
> + * 2.  Redistributions in binary form must reproduce the above copyright
> + *     notice, this list of conditions and the following disclaimer in the
> + *     documentation and/or other materials provided with the distribution.
> + * 3.  Neither the name of Apple Computer, Inc. ("Apple") nor the names of
> + *     its contributors may be used to endorse or promote products derived
> + *     from this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY APPLE AND ITS CONTRIBUTORS "AS IS" AND ANY
> + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED
> + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
> + * DISCLAIMED. IN NO EVENT SHALL APPLE OR ITS CONTRIBUTORS BE LIABLE FOR ANY
> + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
> + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
> + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
> + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#ifndef Vector3_h
> +#define Vector3_h
> +
> +#include <math.h>
> +
> +namespace WebCore {
> +
> +// ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +// Basic 3D vector type

I don't think this comment adds much.

> +
> +    double dot(const Vector3& v) const;
> +
> +    Vector3 cross(const Vector3& v) const;
> +
> +    double distance(const Vector3& v) const;

The parameter names here should be removed.

> +private:
> +    friend double dot(const Vector3& v1, const Vector3& v2);
> +    friend Vector3 cross(const Vector3& v1, const Vector3& v2);
> +    friend Vector3 operator*(double k, const Vector3& v);
> +    friend Vector3 operator+(const Vector3& v1, const Vector3& v2);
> +    friend Vector3 operator-(const Vector3& v1, const Vector3& v2);

Here too.

> +inline Vector3 cross(const Vector3& v1, const Vector3& v2)
> +{
> +    double x3 = v1.m_y * v2.m_z  -  v1.m_z * v2.m_y;
> +    double y3 = v1.m_z * v2.m_x  -  v1.m_x * v2.m_z;
> +    double z3 = v1.m_x * v2.m_y  -  v1.m_y * v2.m_x;
> +    return Vector3(x3, y3, z3);
> +}

Some double spaces here.

r- to fix these nits.

[Chris Rogers]

Created attachment 48160 [details]
address Sam's style comments

[Darin Adler]

Comment on attachment 48160 [details]
address Sam's style comments

Looks good.

> +    Vector3(float* p)

This should be const float*, or actually:

    Vector(const float p[3])

Which documents what the pointer should point to, but is otherwise equivalent.

> +    Vector3(double* p)

Ditto. const double[3] would be best.

> +    // Explicit copy-constructor
> +    Vector3(const Vector3& v)

Why? The compiler will generate exactly this, so why define it explicitly? It's typically considered better style to omit this.

> +    double dot(const Vector3&) const;
> +
> +    Vector3 cross(const Vector3&) const;
> +
> +    double distance(const Vector3&) const;

Do we need these members? Why not just have plain old functions? Is code that uses the member function syntax clearer? Could you show me an example?

> +    friend double dot(const Vector3&, const Vector3&);
> +    friend Vector3 cross(const Vector3&, const Vector3&);
> +    friend Vector3 operator*(double, const Vector3&);
> +    friend Vector3 operator+(const Vector3&, const Vector3&);
> +    friend Vector3 operator-(const Vector3&, const Vector3&);

Why do these need to be friends? They should use the public inline x(), y() and z() functions and so won't need to be friends.

review- for now, but seems generally good

[Chris Rogers]

Created attachment 48168 [details]
address all of Darin's comments

Thanks Darin - all of your comments sounded good

[Oliver Hunt]

Comment on attachment 48168 [details]
address all of Darin's comments

Why is this in WebCore -- this is a primitive type, it's not necessarily specific for Audio (eg. we may want vector3 in the DOM for WebGL, etc) -- it seems this belongs in wtf, provisional r- unless there's a good reason for this.

[Darin Adler]

Seems OK to have it in WebCore/platform although slightly better to have it in JavaScriptCore/wtf.

[Chris Rogers]

Created attachment 48182 [details]
move Vector3.h to wtf directory per Oliver's request

[Darin Adler]

Comment on attachment 48182 [details]
move Vector3.h to wtf directory per Oliver's request

> +    void normalize()
> +    {
> +        if (isZero())
> +            return;
> +
> +        double k = 1.0 / abs();
> +        m_x *= k;
> +        m_y *= k;
> +        m_z *= k;
> +    }

Seems to me it would be more efficient to check the result of abs() for 0.0 rather than separately checking isZero. One equality check against zero instead of three in the normal case.

> +inline Vector3 operator*(double k, const Vector3& v)
> +{
> +    return Vector3(k * v.x(), k * v.y(), k * v.z());
> +}

I think you should also include the version where the vector is on the left.

> +	Vector3 v = v1 - v2;
> +    return v.abs();

There's a stray tab in here on that first line, that needs to be fixed.

You can just write this without the named temporary as

    return (v1 - v2).abs();

Seems a little cleaner like that to me.

r=me but you can’t land until you get rid of that tab.

[Chris Rogers]

Created attachment 48183 [details]
address last of Darin's comments

Thanks Darin, addressed all of your last issues except the first one, since abs() calls sqrt() it's actually *less* efficient than calling isZero()

[Chris Rogers]

Oh, and I don't have committer status, so could somebody land this for me -- thanks!

[Darin Adler]

(In reply to comment #14)
> Thanks Darin, addressed all of your last issues except the first one, since
> abs() calls sqrt() it's actually *less* efficient than calling isZero()

I think you're missing my point.

If isZero is true, then abs is less efficient, but in the normal case where isZero turns out to be false, just checking the result of abs is more efficient. I was not trying to optimize the zero case.

[Chris Rogers]

Created attachment 48186 [details]
last minute optimization to normalize() method

[WebKit Commit Bot]

Comment on attachment 48186 [details]
last minute optimization to normalize() method

Rejecting patch 48186 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Adler', '--force']" exit_code: 2
patching file JavaScriptCore/ChangeLog
patch: **** unexpected end of file in patch
patching file JavaScriptCore/wtf/Vector3.h

Full output: http://webkit-commit-queue.appspot.com/results/236639

[Eric Seidel (no email)]

Yes, looks like the patch simply failed to apply.  Not sure why.  How did you generate the patch?

Note that the EWS bubbles below the review links on the attachment have all been purple.  That means the patch has been failing to apply.

[Chris Rogers]

Created attachment 48352 [details]
fixes problem with previous patch not applying correctly

Sorry - the patch which was approved was not able to apply correctly due to an error on my part
I've attached a correct patch (at least svn-apply works for me locally using it)

There are no changes to the actual contents which was already approved, please try re-committing...

[Eric Seidel (no email)]

Comment on attachment 48182 [details]
move Vector3.h to wtf directory per Oliver's request

Cleared Darin Adler's review+ from obsolete attachment 48182 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Eric Seidel (no email)]

Comment on attachment 48186 [details]
last minute optimization to normalize() method

Obsoleting this patch since a new one has been posted.

[WebKit Commit Bot]

Comment on attachment 48352 [details]
fixes problem with previous patch not applying correctly

Clearing flags on attachment: 48352

Committed r54522: <http://trac.webkit.org/changeset/54522>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.