nus-vv-streams / vvtk

A toolkit for volumetric video research
MIT License
9 stars 8 forks source link

branch-reconstruct, IndexedPointCloud message and PointXyzRgba #33

Closed YoHoSo closed 3 months ago

YoHoSo commented 1 year ago
image

maybe it is better to add a new pipeline message enum for your TriangleFace. e.g.

IndexedPointCloudWithTriangleFace(PointCloud<PointXyzRgba>, u32, Option<Vec<TriangleFace>>)

Since it is not clear yet if TriangleFace will be used elsewhere. Also, adding a new message enum will not interfere with the original functionality.

Let me know your thoughts on this.

YoHoSo commented 1 year ago
image

Also for PointXyzRgba, we have talked about this before over email. It is better to create a new data structure for the normals. Consider changing into this, and implement the from/to so that PointXyzRgbaWithNormal can convert from/to PointXyzRgba

#[repr(C)]
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PointXyzRgbaWithNormal {
    pub x: f32,
    pub y: f32,
    pub z: f32,
    pub r: u8,
    pub g: u8,
    pub b: u8,
    pub a: u8,
    pub nx: f32,
    pub ny: f32,
    pub nz: f32,
}

or alternatively with Option for nx, ny, nz.

#[repr(C)]
#[derive(Debug, Copy, Clone, PartialEq)]
pub struct PointXyzRgbaWithNormal {
    pub x: f32,
    pub y: f32,
    pub z: f32,
    pub r: u8,
    pub g: u8,
    pub b: u8,
    pub a: u8,
    pub nx: Option<f32>,
    pub ny: Option<f32>,
    pub nz: Option<f32>,
}

Let me know your thoughts

YoHoSo commented 1 year ago

Changing the original data structure will generate a lot of incompatibility like this.

image
bokuanT commented 1 year ago

WeiQiang said he has implemented a more proper class to store points containing normals. I plan on using his new class to replace mine after you have approved his finalised version

bokuanT commented 1 year ago

maybe it is better to add a new pipeline message enum for your TriangleFace. e.g.

IndexedPointCloudWithTriangleFace(PointCloud<PointXyzRgba>, u32, Option<Vec<TriangleFace>>)

I think this is better than my current implementation, I will do this

YoHoSo commented 1 year ago

WeiQiang said he has implemented a more proper class to store points containing normals. I plan on using his new class to replace mine after you have approved his finalised version

Sure, let's see his idea later