Skip to content
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.

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

[wpimath] Make a class for a bound vector #7542

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

Conversation

TheComputer314
Copy link

@TheComputer314 TheComputer314 commented Dec 12, 2024

In AdvantageScope, you can visualize a vector in a vector field by using a Pose2d and setting its visualization mode to "Arrow". However, you cannot change the length or color of the arrow, making vector field visualizations using Pose2d arrays cluttered and hard to read.

image

BoundVector2d, which this PR adds, is intended for use in vector field visualizations by having both a position relative to origin, and component vectors relative to the position. This class can also be used to represent a force applied on a mechanism, offset from the center of that mechanism.

TODO:

  • Make C++ impls
  • Add Protobuf serialization
  • Implement visualizations using this class in AScope

@TheComputer314 TheComputer314 requested a review from a team as a code owner December 12, 2024 23:13
@github-actions github-actions bot added the component: wpimath Math library label Dec 12, 2024
Copy link
Member

@calcmogul calcmogul left a comment

Choose a reason for hiding this comment

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

This doesn't feel general enough to belong in wpimath. It's the internal representation of something for a specific tool. I don't see where else one could use this.

* Constructs a vector with the specified position and component vectors.
*
* @param position The position of the vector.
* @param components The X/Y component vectors.
Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by "component vectors" here? This variable is poorly named. The two fields in a Translation2d are scalars, not vectors.

Copy link
Author

Choose a reason for hiding this comment

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

A scalar only has a magnitude while a vector has both direction and magnitude right? The direction can be just the sign of a number, and isn't necessarily an angle. Translation2d.getX() and Translation2d.getY() would be vectors in their respective axes. This needs to be made more clear though.

Copy link
Member

@calcmogul calcmogul Dec 13, 2024

Choose a reason for hiding this comment

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

Translation2d is a 2D vector represented by <x, y>. We don't consider the elements x and y to also be vectors. If we did, what we actually have is a matrix (rank-2 tensor).

x and y in this case represent the magnitudes of the î and ĵ unit vectors.

import edu.wpi.first.util.struct.StructSerializable;
import java.util.Objects;

/** Represents a bound vector in a vector field. Has an origin, and x/y component vectors. */
Copy link
Member

@calcmogul calcmogul Dec 12, 2024

Choose a reason for hiding this comment

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

This should be defining a lot more terms. What's a "bound vector"? What are "component vectors"? When would someone use this class in their robot code?

*
* @return The X component of the vector.
*/
public double getXComponent() {
Copy link
Member

Choose a reason for hiding this comment

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

This isn't really saving much typing over getComponents().getX(). At least Pose2d.getX() is a lot shorter than Pose2d.getTranslation().getX().

Copy link
Author

Choose a reason for hiding this comment

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

I personally think we should still include this, it saves a little bit of typing even if it isn't much.

@PeterJohnson
Copy link
Member

PeterJohnson commented Dec 13, 2024

It feels generic enough, as it’s a standard geometry/physics/engineering thing? https://en.m.wikipedia.org/wiki/Vector_quantity

@calcmogul
Copy link
Member

calcmogul commented Dec 13, 2024

It feels generic enough, as it’s a standard geometry/physics/engineering thing? https://en.m.wikipedia.org/wiki/Vector_quantity

We already have a type for vectors: https://github.com/wpilibsuite/allwpilib/blob/main/wpimath/src/main/java/edu/wpi/first/math/Vector.java

BoundVector2d on the other hand has very limited uses; I can only think of the specific one the OP already mentioned. It belongs in a library published by, say, AdvantageScope, not in wpimath. Lack of general applicability is why several of the utilities SysId uses haven't been moved into wpimath either.

@PeterJohnson
Copy link
Member

A bound vector is not a vector.

@calcmogul
Copy link
Member

calcmogul commented Dec 13, 2024

It's a vector and point of action, which (1) is only useful for this specific situation, and (2) doesn't interoperate with the other geometry types at all. That's probably why C++'s Eigen and Rust's nalgebra don't have one in their collection of geometry types.

@TheComputer314
Copy link
Author

I don't disagree with the fact that the use cases are niche, but it isn't exclusively limited to this case, it could be used to represent an applied force offset from the center of a mechanism.
image
Plus, we already have other geometry classes with use cases that are rather niche for what users do. (Rectangle2d and Ellipse2d)

Also, in regards to naming things, would m_startPoint and m_offsetVector be more clear than m_position and m_components?

@calcmogul
Copy link
Member

it could be used to represent an applied force offset from the center of a mechanism.

When would someone need to do that though? If there is an application, is that the most convenient way to accomplish that?

Plus, we already have other geometry classes with use cases that are rather niche for what users do. (Rectangle2d and Ellipse2d)

They're useful for trajectory region constraints and bounds checks for robot actions or triggers.

@TheComputer314
Copy link
Author

TheComputer314 commented Dec 13, 2024

When would someone need to do that though?

PathPlanner's swerve setpoint generator and maple-sim both use something like this (a force with a start point) to represent force applied from a swerve module to the ground.

If there is an application, is that the most convenient way to accomplish that?

TBH, I'm not 100% sure if it is the most convenient way, but I personally think its up there in terms of convenience to have both the vector and the start point of that vector in one place. I'm not against rewriting this if it means a better way to accomplish the goal.

@calcmogul
Copy link
Member

calcmogul commented Dec 13, 2024

PathPlanner's swerve setpoint generator and maple-sim both use something like this (a force with a start point) to represent force applied from a swerve module to the ground.

It doesn't look like they do. They just have lists of module locations and force magnitudes. I'm also not seeing where a BoundVector type would clean anything up.

https://github.com/mjansen4857/pathplanner/blob/main/pathplannerlib/src/main/java/com/pathplanner/lib/util/swerve/SwerveSetpointGenerator.java
https://github.com/Shenzhen-Robotics-Alliance/maple-sim/blob/main/project/src/main/java/org/ironmaple/simulation/drivesims/SwerveDriveSimulation.java
https://github.com/Shenzhen-Robotics-Alliance/maple-sim/blob/main/project/src/main/java/org/ironmaple/simulation/drivesims/SwerveModuleSimulation.java

I personally think its up there in terms of convenience to have both the vector and the start point of that vector in one place

If you like it, then put it in the source code of your app that uses it. It doesn't belong in wpimath though.

@calcmogul
Copy link
Member

The new names are an improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants