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

Introduce JointArray::{map, zip} #50

Merged
merged 12 commits into from
Aug 26, 2023

Conversation

jkbkaiser
Copy link
Contributor

Implemented necessary traits.

nidhogg/src/types.rs Outdated Show resolved Hide resolved
nidhogg/src/types.rs Outdated Show resolved Hide resolved
@jkbkaiser jkbkaiser changed the title IntoIter and FromIter for JointArray JointArray iteration Aug 15, 2023
@YukumoHunter
Copy link
Member

YukumoHunter commented Aug 15, 2023

My €‎0,02:

I think the Iterator trait is better suited semantically to collections of arbitrary size and without any positional information (every index tied to a specific joint). Point in case: A lot of methods that are auto-implemented don't really make sense in our case (what does it mean to .filter() or .is_sorted() on a JointArray?). This also probably explains the frustrations with the infallible FromIterator causing us to have to unwrap or use a default value.

I think the primary use case was being able to easily zip two JointArrays/map over all the values? In which case I suggest implementing a sole zip and map method on JointArray<T>:

pub fn map<F, U>(self, mut f: F) -> JointArray<U>
    where
        F: FnMut(T) -> U,
    {
        JointArray {
            head_yaw: f(self.head_yaw),
            head_pitch: f(self.head_pitch),
            left_shoulder_pitch: f(self.left_shoulder_pitch),
            left_shoulder_roll: f(self.left_shoulder_roll),
            left_elbow_yaw: f(self.left_elbow_yaw),
            left_elbow_roll: f(self.left_elbow_roll),
            left_wrist_yaw: f(self.left_wrist_yaw),
            left_hip_yaw_pitch: f(self.left_hip_yaw_pitch),
            left_hip_roll: f(self.left_hip_roll),
            left_hip_pitch: f(self.left_hip_pitch),
            left_knee_pitch: f(self.left_knee_pitch),
            left_ankle_pitch: f(self.left_ankle_pitch),
            left_ankle_roll: f(self.left_ankle_roll),
            right_shoulder_pitch: f(self.right_shoulder_pitch),
            right_shoulder_roll: f(self.right_shoulder_roll),
            right_elbow_yaw: f(self.right_elbow_yaw),
            right_elbow_roll: f(self.right_elbow_roll),
            right_wrist_yaw: f(self.right_wrist_yaw),
            right_hip_roll: f(self.right_hip_roll),
            right_hip_pitch: f(self.right_hip_pitch),
            right_knee_pitch: f(self.right_knee_pitch),
            right_ankle_pitch: f(self.right_ankle_pitch),
            right_ankle_roll: f(self.right_ankle_roll),
            left_hand: f(self.left_hand),
            right_hand: f(self.right_hand),
        }
    }

and similarly for zip:

pub fn zip<U>(self, other: JointArray<U>) -> JointArray<(T, U)> { ... }

This way we can modify JointArrays without any implicit cloning or conversion between iterator types and back again. Having a sole map method is also done in the std implementation of array (and a zip method is usable in nightly) so I don't think it's a weird thing to do either!

Would be glad to hear your opinions @Redjeepkaiser @Aemulation

@Aemulation
Copy link
Contributor

If map and zip are the only reasons @Redjeepkaiser wanted to implement an iterator, I agree with implementing only those traits, it should be better for the performance as well.

But I am wondering, what is the reason we need a zip function?

@YukumoHunter
Copy link
Member

If map and zip are the only reasons @Redjeepkaiser wanted to implement an iterator, I agree with implementing only those traits, it should be better for the performance as well.

But I am wondering, what is the reason we need a zip function?

zipping two JointArrays together is useful when you need to interpolate between them:

pos1
    .zip(pos2)
    .map(|(p1, p2)| 0.5 * p1 + 0.5 * p2)

nidhogg/src/types.rs Outdated Show resolved Hide resolved
Copy link
Member

@oxkitsune oxkitsune left a comment

Choose a reason for hiding this comment

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

Everything looks good to me!
Suggested commit message:

Introduce `JointArray::{map, zip}`

@oxkitsune oxkitsune changed the title JointArray iteration Introduce JointArray::{zip, map} Aug 26, 2023
@oxkitsune oxkitsune changed the title Introduce JointArray::{zip, map} Introduce JointArray::{map, zip} Aug 26, 2023
@Aemulation Aemulation added the enhancement New feature or request label Aug 26, 2023
@oxkitsune oxkitsune merged commit 68889d8 into main Aug 26, 2023
5 checks passed
@oxkitsune oxkitsune deleted the jkaiser/jointarray_vector_manipulation branch August 26, 2023 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants