Skip to content
This repository has been archived by the owner on Feb 22, 2024. It is now read-only.

Add Point2D library #6

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Add Point2D library #6

wants to merge 6 commits into from

Conversation

joseviccruz
Copy link
Member

This pull request introduces the Point2D library, providing a versatile and efficient implementation of a 2D point structure. The Point2D library offers various mathematical operations and geometric functions that can be utilized in a wide range of applications.

@joseviccruz joseviccruz added the enhancement New feature or request label Jun 15, 2023
@joseviccruz joseviccruz self-assigned this Jun 15, 2023
Comment on lines 90 to 91
- `value_type dot(const Point2D& other) const`: Computes the dot product of the current point and another point.
- `value_type cross(const Point2D& other) const`: Computes the cross product of the current point and another point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dot and cross are common vectors operations. Some operations are specific for vector or point interpretation, hardly for both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Should I change here in the description only? Or implement Point / Vector separately?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Just description I think.

- `value_type manhattanLength() const`: Computes the Manhattan distance between the origin and the current point.
- `value_type lengthSquared() const`: Computes the square of the Euclidean length of the current point.
- `auto length() const`: Computes the Euclidean length of the current point.
- `auto norm() const`: Computes the Euclidean length of the current point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

A point doesn't have length.


- `value_type dot(const Point2D& other) const`: Computes the dot product of the current point and another point.
- `value_type cross(const Point2D& other) const`: Computes the cross product of the current point and another point.
- `value_type manhattanLength() const`: Computes the Manhattan distance between the origin and the current point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This a example of point only operation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont like that. So for manhattan that commonly a binary function, I have to do:
(b-a).manhattanLength()
Instead of:
a.manhattanDistance(b)?
I think that manhattan is more related to distance than length.

Copy link
Member Author

Choose a reason for hiding this comment

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

Very well observed! Again, this was partly inspired by QPointF::manhattanLength.

Perhaps keeping both is reasonable?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes.

robocin/geometry/README.md Outdated Show resolved Hide resolved
Comment on lines 100 to 101
- `auto angle() const`: Computes the angle (in radians) between the positive x-axis and the vector from the origin to
the current point.
Copy link
Collaborator

Choose a reason for hiding this comment

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

[-Pi , +PI]?


### Validators

- `bool isNull() const`: Checks if the point is null (coordinates are both zero). Returns `true`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- `bool isNull() const`: Checks if the point is null (coordinates are both zero). Returns `true`
- `bool isOrigin() const`: Checks if the point is null (coordinates are both zero). Returns `true`

discard null word for this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was originally inspired on QPointF::isNull, but I agree that isOrigin looks better for this purpose...

However, perhaps isNull is a common interface for other geometric representations such as QRectF, QVector3D...

wdyt?

Comment on lines 80 to 82
- `bool operator==(const Point2D& other) const`: Equality operator that checks if two points are equal.
- `auto operator<=>(const Point2D& other) const`: Three-way comparison operator that compares two points and returns
their relative ordering.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Are these operators sufficient to do sorting?

Copy link
Member Author

Choose a reason for hiding this comment

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

For sure! Three way comparisons are able to replace all relational comparators.

@joseviccruz joseviccruz requested a review from bcs5 July 31, 2023 22:04
@joseviccruz joseviccruz marked this pull request as ready for review September 17, 2023 18:55
@joseviccruz joseviccruz changed the base branch from fuzzy_compare to main September 17, 2023 18:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

2 participants