Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion sdformat/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -182,7 +182,60 @@ pub struct GraspCheck {
pub struct Actor {}
/// The light element describes a light sou
#[derive(Debug, Serialize, Deserialize)]
pub struct Light {}
pub struct Light {
// A unique name for the light

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Please use doc comments /// so that the information shows in the documentation.

name: String,
// The light type: point, directional spot

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • there is a missing comma point, directional, spot

#[serde(default)]

@elpiel elpiel Oct 2, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • The default should be set up using serde.
  • Use an Enum since the available options are only 3 point, directional, spot
    You might need to use https://crates.io/crates/parse-display for this. I'm not aware how Enums with no data are deserialized so you have to double-check this.
  • Make sure to rename the light_type to type with an serde attribute

light_type: String,
// When true, the light will cast shadows
#[serde(default)]
cast_shadows: Option<bool>,
// Scale factor to set the relative power of a light
#[serde(default)]
intensity: Option<f64>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • The default for this value is 1, right now with serde(default) it is None.

// Diffuse light color
#[serde(default)]
diffuse: Option<Color>,
// Specular light color
#[serde(default)]
specular: Option<Color>,
Comment on lines +199 to +202

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Please add default functions that set the correct values for diffuse (1 1 1 1) & specular (0.1 0.1 0.1 1). This could be achieved by adding associated functions to Color for the default values, like Color::specular_default()

// Light attenuation
#[serde(default)]
attenuation: Vec<Attenuation>,
// Direction of light, only applicable for spot and directional light
#[serde(default)]
direction: Vec<i32>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Vec3 actually means a mathematical vector.
You can use a tuple struct and eventually we might use something like nalgebra's structs but this is not required for the moment

https://docs.rs/nalgebra/0.29.0/nalgebra/base/type.Vector3.html

  • Please add proper default.

// Spot light parameters
#[serde(default)]
spot: Vec<Spot>,
// A position and orientation with respect to the frame named in relative_to attribute
#[serde(default)]
pose: Vec<Pose>,
}

// Light attenuation
pub struct Attenuation {
range: f64,
linear: Option<f64>,
constant: Option<f64>,
quadratic: Option<f64>,
}

// Spot light parameters
pub struct Spot {
inner_angle: f64,
outer_angle: f64,
falloff: f64,
}

// A position and orientation with respect to the frame named in relative_to attribute
pub struct Pose {
relative_to: Option<String>,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • Pose has value as well
    <pose relative_to="...">value</pose>

}

// Unit struct for tpe `color`
struct Color(u8, u8, u8);
Comment on lines +218 to +238

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  • structs are missing the documentation of the fields.
  • derive Debug, Deser. & Ser.


#[cfg(test)]
mod test {
Expand Down