Skip to content

catch for vector parallel to normal in angle_vectors_projected #1462

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
+4 −0
Open

catch for vector parallel to normal in angle_vectors_projected #1462

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2c0e481
Merge branch 'main' of https://github.com/obucklin/compas
obucklin Apr 7, 2025
30f7d0e
Merge branch 'main' of https://github.com/compas-dev/compas
obucklin May 15, 2025
5b10b45
added catch for vector parallel to normal
obucklin May 15, 2025
1af2b03
Changelog
obucklin May 15, 2025
b75d2d7
changed to raise ValueEror
obucklin Jun 11, 2025
6970beb
Merge branch 'main' of https://github.com/compas-dev/compas
obucklin Jun 11, 2025
0597af4
Merge branch 'main' into fix_angle_vectors_projected
obucklin Jun 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
* Fixed a bug when printing an empty `Tree`.
* Fixed a bug in `Group` for IronPython where the decoding declaration was missing.
* Fixed a bug where a `Group` without name could not be added to the scene.
* Fixed a bug where `angle_vectors_projected` returned a 0 when an input vector was parallel to projection normal. Now returns `None`.

### Removed

Expand Down
4 changes: 2 additions & 2 deletions src/compas/files/gltf/gltf_node.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def rotation(self, value):
if self._matrix:
raise Exception("Cannot set rotation when matrix is set.")
if not isinstance(value, list) or len(value) != 4 or fabs(sum([q**2 for q in value]) - 1) > 1e-03:
raise Exception("Invalid rotation. Rotations are expected to be given as " "unit quaternions of the form [q1, q2, q3, q4]")
raise Exception("Invalid rotation. Rotations are expected to be given as unit quaternions of the form [q1, q2, q3, q4]")
self._rotation = value

@property
Expand Down Expand Up @@ -199,7 +199,7 @@ def matrix(self, value):
raise Exception("Invalid matrix. A 4x4 matrix is expected.")
if value[3] != [0, 0, 0, 1]:
raise Exception(
"Invalid matrix. A matrix without shear or skew is expected. It must be of " "the form TRS, where T is a translation, R is a rotation and S is a scaling."
"Invalid matrix. A matrix without shear or skew is expected. It must be of the form TRS, where T is a translation, R is a rotation and S is a scaling."
)
self._matrix = value

Expand Down
3 changes: 3 additions & 0 deletions src/compas/geometry/_core/angles.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ def angle_vectors_projected(u, v, normal, deg=False, tol=None):
u_cross = cross_vectors(u, normal)
v_cross = cross_vectors(v, normal)

if TOL.is_allclose(u_cross, [0.0, 0.0, 0.0]) or TOL.is_allclose(v_cross, [0.0, 0.0, 0.0]):
Copy link
Member

@chenkasirer chenkasirer Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh just a thought here, I'm not sure this is a bug in this particular function. like if you pass u or v such that one or both of them are parallel to normal the angle between u and v projected on the plane IS in-fact 0, right?

from an API perspective (well mine ;) we'd be forcing angle_vectors_projected here to communicate to us something it's not necessarily concerned with.

Copy link
Member

@chenkasirer chenkasirer Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

of course if projecting v onto plane with normal v is mathematically undefined then I'd perhaps raise ValueError here rather than return None, for explicity's sake

Copy link
Member

@tomvanmele tomvanmele Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree with @chenkasirer

the angle between two vectors can't be None.
the function should either return a float or throw an error.

and it should only throw an error if the angle cannot be computed form the input data, not because the result value is "inconvenient" :)

Copy link
Member

@tomvanmele tomvanmele Jun 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the project onto the plane of one of the two vectors is a vector of zero length, then the angle wrt that plane is indeed 0 in my opinion

Copy link
Contributor

@papachap papachap Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

returning 0 when one of the vectors is zero-length is more of a convention, right? since in strict math terms the angle is undefined (division by zero)

this is more an argument for throwing an error, unless it's a common convention that i am just not familiar with?

Copy link
Member

@chenkasirer chenkasirer Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no mathematician but according to the discussion here the angle between a vector and a zero vector is indeed undefined, as @papachap said. unfortunately no linear algebra books around me but if this is correct then i guess it supports raising an error, which should maybe done here?:

compas/src/compas/geometry/_core/angles.py

Lines 47 to 49 in ba669cb

L = length_vector(u) * length_vector(v)
if TOL.is_zero(L, tol):
return 0

Copy link
Contributor Author

@obucklin obucklin Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, returning 0 is clearrly incorrect, as that is a valid float indicating parallel vectors. From the comments, it seems raising a ValueError seems to be the way to go.

Copy link
Member

@tomvanmele tomvanmele Jun 4, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you perhaps post a snippet in which this specific case would be triggered, and how it is ideally handled? would you indeed expect the code to stop, and add an error catching block to handle this?

Copy link
Contributor Author

@obucklin obucklin Jun 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the current usage:

inclination = angle_vectors_projected(zzaxis, front_plane.normal, yyaxis)
if inclination is None:
    inclination = angle_vectors_signed(zzaxis, ref_side.xaxis, ref_side.normal, deg=True)

I would go ahead and change this to:

if is_parallel_vector_vector(zzaxis, front_plane.normal) or is_parallel_vector_vector(zzaxis, yyaxis):
    inclination = angle_vectors_signed(zzaxis, ref_side.xaxis, ref_side.normal, deg=True)
else:
    inclination = angle_vectors_projected(zzaxis, front_plane.normal, yyaxis)

I don't expect any particular behavior, other than it should not return 0. I could imagine that being mathematically undefined, as stated by @papachap, could equate to returning a None. However it seems that raising an error is preferred. I will go ahead and make that change for y'all's consideration.

return None

return angle_vectors_signed(u_cross, v_cross, normal, deg, tol)


Expand Down
2 changes: 1 addition & 1 deletion src/compas/rpc/dispatcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ def _dispatch(self, name, args):
try:
idict = json.loads(args[0], cls=DataDecoder)
except (IndexError, TypeError):
odict["error"] = "API methods require a single JSON encoded dictionary as input.\n" "For example: input = json.dumps({'param_1': 1, 'param_2': [2, 3]})"
odict["error"] = "API methods require a single JSON encoded dictionary as input.\nFor example: input = json.dumps({'param_1': 1, 'param_2': [2, 3]})"

else:
self._call(function, idict, odict)
Expand Down
Loading