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

pasture_algorithms require POSITION_3D attribute to be f64. #23

Open
dan-jfisher opened this issue Nov 13, 2023 · 4 comments
Open

pasture_algorithms require POSITION_3D attribute to be f64. #23

dan-jfisher opened this issue Nov 13, 2023 · 4 comments

Comments

@dan-jfisher
Copy link

It would be nice to generalize this to f32 as well.

My first thought was to accept an AttributeIterator directly in functions like compute_centroid. But that doesn't seem like a good solution.

Maybe it would be useful to have a super trait of PrimitiveType for Vector3<f64> and Vector3<f32>?

I would much rather contribute here than wrap pcl, so I am happy to work on this if necessary.

@dan-jfisher
Copy link
Author

dan-jfisher commented Nov 13, 2023

Here is an example of what I did to get the behavior I wanted for compute_centroid.

pub fn compute_centroid<'a, T, F>(
	point_cloud: &'a T
) -> Vector3<F>
where
    T: BorrowedBuffer<'a>,
    F: Float + RealField,
    Vector3<F>: PrimitiveType,
{
    if point_cloud.is_empty() {
        panic!("The point cloud is empty!");
    }

    let mut centroid = Vector3::<F>::zeros();
    let mut temp_centroid = Vector3::<F>::zeros();

    if is_dense(point_cloud) {
        // add all points up
        for point in point_cloud.view_attribute::<Vector3<F>>(&POSITION_3D) {
            temp_centroid[0] += point.x;
            temp_centroid[1] += point.y;
            temp_centroid[2] += point.z;
        }

        //normalize over all points
        centroid[0] = temp_centroid[0] / F::from(point_cloud.len()).unwrap();
        centroid[1] = temp_centroid[1] / F::from(point_cloud.len()).unwrap();
        centroid[2] = temp_centroid[2] / F::from(point_cloud.len()).unwrap();
    } else {
        let mut points_in_cloud = 0;
        for point in point_cloud.view_attribute::<Vector3<F>>(&POSITION_3D) {
            if is_finite(&point) {
                // add all points up
                temp_centroid[0] += point.x;
                temp_centroid[1] += point.y;
                temp_centroid[2] += point.z;
                points_in_cloud += 1;
            }
        }

        // normalize over all points
        centroid[0] = temp_centroid[0] / F::from(points_in_cloud).unwrap();
        centroid[1] = temp_centroid[1] / F::from(points_in_cloud).unwrap();
        centroid[2] = temp_centroid[2] / F::from(points_in_cloud).unwrap();
    }

    centroid
}

It would obviously be nice if we could check whether or not the attribute has the right format at compile time, but I don't think that is possible with the current layout structure.

@Mortano
Copy link
Collaborator

Mortano commented Nov 14, 2023

I agree that the algorithms are not particularly generic at the moment. What you describe is a general theme in pasture due to the fact that PointLayout essentially encodes a type at runtime. So no compile-time checks for layouts, because there are many situations where the layout is not known at compile-time (think reading LAS files with their different point records).

That being said, would the converting attribute views be an option for you? I.e. point_cloud.view_attribute_with_conversion::<Vector3<f64>>(&POSITION_3D). The returned centroid would be Vector3<f64>, but the function itself would work with any point cloud that has a positional attribute that is convertible to Vector3<f64>. You can take a look at calculate_bounds for an example.

If this works for you, I'd be happy to take a PR for this :)

@dan-jfisher
Copy link
Author

I appreciate the quick response :)

For context: my company, Fizyr, is in the process of porting our codebase to Rust. If possible, we want to replace our PCL ffi wrappers with Pasture. We are more than happy to contribute if you are open to the features that we would need:

For functions that are going to iterate over every point in the pointcloud, we need the option to run them with f32 rather than converting everything using the AttributeViewConverting.

We would also need a fallible view_attribute_checked function that returns an error rather than panicking if the attribute's type does not match the requested one.

If you have another communication channel where you want to discuss this further, feel free to email me the details. My email is in my bio.

@Mortano
Copy link
Collaborator

Mortano commented Nov 15, 2023

I sent you a mail :)

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

No branches or pull requests

2 participants