`GfPlane::_distance` appears to have the wrong sign or docs could use some more clarification

Description of Issue

According to this comment block, the equation of a plane in GfPlane is in standard form :

Lines 66 to 70 in 9b0c13b

    
           /// This constructor creates a plane given by the equation  
        
           /// \p eqn[0] * x + \p eqn[1] * y + \p eqn[2] * z + \p eqn[3] = 0. 
        
           GfPlane(const GfVec4d &eqn) { 
        
               Set(eqn); 
        
           }

Since all constructors normalize the incoming normal vector, we can further assume that GfPlane is in Hessian Normal Form. We can derive that form here.

$$\begin{align*} \mathbf{n}^T(\mathbf{x} - \mathbf{x_0}) &= 0 \\ A(x - x_0) + B(y - y_0) + C(z - z_0) &= 0 \\ Ax - Ax_0 + By - By_0 + Cz - Cz_0 &= 0 \\ Ax + By + Cz + D &= 0 \\ \frac{1}{\sqrt{A^2 + B^2 + C^2}} (Ax + By + Cz + D) &= 0 \\ ax + by + cz + d &= 0 \\ \hat{\mathbf{n}}^T\mathbf{x} + d &= 0 \\ \hat{\mathbf{n}}^T\mathbf{x} &= -d \end{align*}$$

For brevity, we can map the code to the variables above : eqn[0] = $a$, eqn[1] = $b$, eqn[2] = $c$, and eqn[3] = $d$.

The following block expands the vector of coefficients eqn but sets _distance = -eqn[3] = $-d$.

OpenUSD/pxr/base/gf/plane.cpp

Lines 44 to 56 in 9b0c13b

    
           void 
        
           GfPlane::Set(const GfVec4d &eqn) 
        
           { 
        
               for (size_t i = 0; i < 3; i++) { 
        
                   _normal[i] = eqn[i]; 
        
               } 
        
               _distance = -eqn[3]; 
        
               const double l = _normal.Normalize(); 
        
               if (l != 0.0) { 
        
                   _distance /= l; 
        
               } 
        
           }

We can derive the point $\mathbf{p}$ to plane distance $D$, assuming $\mathbf{x}$ is some point on the plane :

$$\begin{align*} D &= \hat{\mathbf{n}}^T(\mathbf{p} - \mathbf{x}) \\ &= \hat{\mathbf{n}}^T\mathbf{p} - \hat{\mathbf{n}}^T\mathbf{x} \\ &= \hat{\mathbf{n}}^T\mathbf{p} - (-d) \\ &= \hat{\mathbf{n}}^T\mathbf{p} + d \end{align*}$$

Plugging in $\mathbf{p} = \mathbf{0}$, we get that the origin to plane distance is actually $+d$ and not $-d$ like the code suggests.

I was curious if this is a bug or an undocumented sign convention. All other "distance" functions use -_distance so I believe GfPlane is self consistent. The only one that actually returns $-d$ is GfPlane::GetDistanceFromOrigin.

Steps to Reproduce

N/A

System Information (OS, Hardware)

N/A

Package Versions

N/A

Build Flags

N/A

If you look at the some of the other Set functions

void
GfPlane::Set(const GfVec3d &normal, const GfVec3d &point)
{
    _normal = normal.GetNormalized();
    _distance = GfDot(_normal, point);
}

void
GfPlane::Set(const GfVec3d &p0, const GfVec3d &p1, const GfVec3d &p2)
{
    _normal = GfCross(p1 - p0, p2 - p0).GetNormalized();
    _distance = GfDot(_normal, p0);
}

void
GfPlane::Set(const GfVec4d &eqn)
{
    for (size_t i = 0; i < 3; i++) {
        _normal[i] = eqn[i];
    }
    _distance = -eqn[3];

    const double l = _normal.Normalize();
    if (l != 0.0) {
        _distance /= l;
    }
}

We can see that _distance follows the convention that a positive distance represents the side of the plane in the direction of the normal vector. As you mentioned, GfPlane is self consistent in this regard.

In the GfVec4 case, the plane equation is given as Ax+By+Cz+D=0. To store the distance in the conventional signed form,
D (aka eqn[3]) is moved to the other side of the equation and negated. So, the signed distance is consistent with the convention used by the other Set functions.

I see your point about clarity, perhaps an additional comment on that Set could mention that the plane is stored as

{ eqn[0], eqn[1],+ eqn[2], distance } and therefore distance is -eqn[3].

Thanks so much for the quick response - I think I understand what the code is doing now.

To rephrase a little : _distance is always the distance along the normal vector.

In standard form, the distance from the origin of a plane defined by $(\hat{\mathbf{n}}, \mathbf{p})$ is :

$$\begin{align*} D &= \hat{\mathbf{n}}^T(\mathbf{0} - \mathbf{p}) \\ &= -\hat{\mathbf{n}}^T\mathbf{p} \\ &= -\cos\theta \\ &= d \\ &= \Rightarrow d \geq 0,\text{if point is facing away from the normal}\\ \end{align*}$$

This is undesirable for graphics applications because it's more intuitive to think about distance along the plane normal to be positive. The standard form considers the distance away from the plane normal to be positive (ie : the plane is running away from the origin).

OpenUSD resolves this with a slight reformulation of the plane equation :

$$\begin{align*} ax + by + cz - d &= 0 \\ \hat{\mathbf{n}}^T\mathbf{x} &= d \end{align*}$$

So that now when we try to find the point to plane distance :

$$\begin{align*} D &= \hat{\mathbf{n}}^T(\mathbf{0} - \mathbf{p}) \\ &= -\hat{\mathbf{n}}^T\mathbf{p} \\ &= -\cos\theta \\ &= -d \\ &= \Rightarrow d \geq 0,\text{if point is facing towards the normal}\\ \end{align*}$$

Like you said, I think the confusion can be resolved with just a single comment, so that those who are expecting the standard form know not to look for it :

- /// \p eqn[0] * x + \p eqn[1] * y + \p eqn[2] * z + \p eqn[3] = 0.
+ /// \p eqn[0] * x + \p eqn[1] * y + \p eqn[2] * z - \p eqn[3] = 0.

Thanks again for your help!

Filed as internal issue #USD-10460

	/// This constructor creates a plane given by the equation
	/// \p eqn[0] * x + \p eqn[1] * y + \p eqn[2] * z + \p eqn[3] = 0.
	GfPlane(const GfVec4d &eqn) {
	Set(eqn);
	}

	void
	GfPlane::Set(const GfVec4d &eqn)
	{
	for (size_t i = 0; i < 3; i++) {
	_normal[i] = eqn[i];
	}
	_distance = -eqn[3];

	const double l = _normal.Normalize();
	if (l != 0.0) {
	_distance /= l;
	}
	}