Bug 34548 - audio engine: add Vector3 class
Summary: audio engine: add Vector3 class
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-03 17:17 PST by Chris Rogers
Modified: 2010-02-08 18:31 PST (History)
9 users (show)

See Also:


Attachments
patch for 3D vector class (5.72 KB, patch)
2010-02-03 17:24 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
fix minor style violations (5.70 KB, patch)
2010-02-03 17:37 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
change filename from VectorTypes.h to Vector3.h (5.67 KB, patch)
2010-02-03 17:57 PST, Chris Rogers
sam: review-
Details | Formatted Diff | Diff
address Sam's style comments (5.52 KB, patch)
2010-02-04 12:17 PST, Chris Rogers
darin: review-
Details | Formatted Diff | Diff
address all of Darin's comments (4.73 KB, patch)
2010-02-04 13:56 PST, Chris Rogers
oliver: review-
Details | Formatted Diff | Diff
move Vector3.h to wtf directory per Oliver's request (4.66 KB, patch)
2010-02-04 17:05 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
address last of Darin's comments (4.75 KB, patch)
2010-02-04 17:22 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
last minute optimization to normalize() method (4.79 KB, patch)
2010-02-04 17:49 PST, Chris Rogers
no flags Details | Formatted Diff | Diff
fixes problem with previous patch not applying correctly (4.80 KB, patch)
2010-02-08 12:34 PST, Chris Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Rogers 2010-02-03 17:17:48 PST
This is a class for a 3D vector, implementing such methods as dot and cross product.
It's needed for 3D audio spatialization.
Comment 1 Chris Rogers 2010-02-03 17:24:42 PST
Created attachment 48082 [details]
patch for 3D vector class
Comment 2 WebKit Review Bot 2010-02-03 17:28:18 PST
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.
Comment 3 Chris Rogers 2010-02-03 17:37:09 PST
Created attachment 48084 [details]
fix minor style violations
Comment 4 Mark Rowe (bdash) 2010-02-03 17:46:55 PST
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.
Comment 5 Chris Rogers 2010-02-03 17:57:58 PST
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...
Comment 6 Sam Weinig 2010-02-04 12:04:04 PST
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 @@
> +2010-02-03  Chris Rogers  <crogers@google.com>
> +
> +        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  <adele@apple.com>
>  
>          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.
Comment 7 Chris Rogers 2010-02-04 12:17:54 PST
Created attachment 48160 [details]
address Sam's style comments
Comment 8 Darin Adler 2010-02-04 13:05:07 PST
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
Comment 9 Chris Rogers 2010-02-04 13:56:52 PST
Created attachment 48168 [details]
address all of Darin's comments

Thanks Darin - all of your comments sounded good
Comment 10 Oliver Hunt 2010-02-04 16:40:52 PST
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.
Comment 11 Darin Adler 2010-02-04 16:50:13 PST
Seems OK to have it in WebCore/platform although slightly better to have it in JavaScriptCore/wtf.
Comment 12 Chris Rogers 2010-02-04 17:05:16 PST
Created attachment 48182 [details]
move Vector3.h to wtf directory per Oliver's request
Comment 13 Darin Adler 2010-02-04 17:10:51 PST
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.
Comment 14 Chris Rogers 2010-02-04 17:22:55 PST
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()
Comment 15 Chris Rogers 2010-02-04 17:24:33 PST
Oh, and I don't have committer status, so could somebody land this for me -- thanks!
Comment 16 Darin Adler 2010-02-04 17:37:03 PST
(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.
Comment 17 Chris Rogers 2010-02-04 17:49:09 PST
Created attachment 48186 [details]
last minute optimization to normalize() method
Comment 18 WebKit Commit Bot 2010-02-04 19:17:33 PST
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
Comment 19 Eric Seidel (no email) 2010-02-05 16:41:58 PST
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.
Comment 20 Chris Rogers 2010-02-08 12:34:12 PST
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...
Comment 21 Eric Seidel (no email) 2010-02-08 15:10:25 PST
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.
Comment 22 Eric Seidel (no email) 2010-02-08 15:23:31 PST
Comment on attachment 48186 [details]
last minute optimization to normalize() method

Obsoleting this patch since a new one has been posted.
Comment 23 WebKit Commit Bot 2010-02-08 18:31:27 PST
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>
Comment 24 WebKit Commit Bot 2010-02-08 18:31:38 PST
All reviewed patches have been landed.  Closing bug.