From 9304ecab3dd6fb07ce8597be5bfbdcf2840ce4fc Mon Sep 17 00:00:00 2001 From: Avi Weinstock Date: Sun, 14 Mar 2021 22:36:59 -0400 Subject: [PATCH] Address Imbris's MR 1888 comments, and add changelog entry. - Use Ori::{new,to_quat} and make the field private. - Update/capitalize/add various comments. - Implicitly drop scope guards where applicable. - Take !Copy colliders by reference instead of cloning. - s/cylinder_voxel_collision/box_voxel_collision/ - Unindent some physics code with a continue. --- CHANGELOG.md | 1 + common/net/src/sync/interpolation.rs | 6 +- common/src/comp/body/ship.rs | 7 +- common/src/comp/ori.rs | 2 +- common/src/states/utils.rs | 1 + common/src/util/find_dist.rs | 2 +- common/sys/src/phys.rs | 177 +++++++++++++-------------- server/src/events/inventory_manip.rs | 2 +- server/src/sys/entity_sync.rs | 2 +- voxygen/src/session.rs | 8 +- 10 files changed, 104 insertions(+), 104 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b369b19bf7..74995d905c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Gave weapons critical strike {chance, multiplier} stats - A system to add glow and reflection effects to figures (i.e: characters, armour, weapons, etc.) - Merchants will trade wares with players +- Airships that can be mounted and flown, and also walked on (`/airship` admin command) ### Changed diff --git a/common/net/src/sync/interpolation.rs b/common/net/src/sync/interpolation.rs index 16e004d30a..2b2207ad09 100644 --- a/common/net/src/sync/interpolation.rs +++ b/common/net/src/sync/interpolation.rs @@ -141,15 +141,15 @@ impl InterpolatableComponent for Ori { return self; } let lerp_factor = 1.0 + ((t2 - t1) / (t1 - t0)) as f32; - let mut out = Slerp::slerp_unclamped(p0.0, p1.0, lerp_factor); + let mut out = Slerp::slerp_unclamped(p0.to_quat(), p1.to_quat(), lerp_factor); if out.into_vec4().map(|x| x.is_nan()).reduce_or() { warn!( "interpolation output is nan: {}, {}, {:?}", t2, lerp_factor, buf ); - out = p1.0; + out = p1.to_quat(); } - Ori(Slerp::slerp(self.0, out, PHYSICS_VS_EXTRAPOLATION_FACTOR).normalized()) + Ori::new(Slerp::slerp(self.to_quat(), out, PHYSICS_VS_EXTRAPOLATION_FACTOR).normalized()) } } diff --git a/common/src/comp/body/ship.rs b/common/src/comp/body/ship.rs index ceab9d86a1..a4a465ccf8 100644 --- a/common/src/comp/body/ship.rs +++ b/common/src/comp/body/ship.rs @@ -90,8 +90,9 @@ pub mod figuredata { let mut colliders = HashMap::new(); for (_, spec) in (manifest.read().0).0.iter() { for bone in [&spec.bone0, &spec.bone1, &spec.bone2].iter() { - // TODO: avoid the requirement for symlinks in "voxygen.voxel.object.", and load - // the models from "server.voxel." instead + // TODO: Currently both client and server load models and manifests from + // "server.voxel.". In order to support CSG procedural airships, we probably + // need to load them in the server and sync them as an ECS resource. let vox = cache.load::(&["server.voxel.", &bone.central.0].concat())?; let dyna = Dyna::::from_vox(&vox.read().0, false); @@ -117,7 +118,7 @@ pub mod figuredata { } lazy_static! { - // TODO: load this from the ECS as a resource, and maybe make it more general than ships + // TODO: Load this from the ECS as a resource, and maybe make it more general than ships // (although figuring out how to keep the figure bones in sync with the terrain offsets seems // like a hard problem if they're not the same manifest) pub static ref VOXEL_COLLIDER_MANIFEST: AssetHandle = AssetExt::load_expect("server.manifests.ship_manifest"); diff --git a/common/src/comp/ori.rs b/common/src/comp/ori.rs index de9e969bcc..351db07066 100644 --- a/common/src/comp/ori.rs +++ b/common/src/comp/ori.rs @@ -9,7 +9,7 @@ use vek::{Quaternion, Vec2, Vec3}; #[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)] #[serde(into = "SerdeOri")] #[serde(from = "SerdeOri")] -pub struct Ori(pub Quaternion); +pub struct Ori(Quaternion); impl Default for Ori { /// Returns the default orientation (no rotation; default Dir) diff --git a/common/src/states/utils.rs b/common/src/states/utils.rs index 545232fa20..42adff3928 100644 --- a/common/src/states/utils.rs +++ b/common/src/states/utils.rs @@ -174,6 +174,7 @@ impl Body { } } + /// Returns flying speed if the body type can fly, otherwise None pub fn can_fly(&self) -> Option { match self { Body::BirdMedium(_) | Body::Dragon(_) | Body::BirdSmall(_) => Some(1.0), diff --git a/common/src/util/find_dist.rs b/common/src/util/find_dist.rs index fc921bc758..10399a2b63 100644 --- a/common/src/util/find_dist.rs +++ b/common/src/util/find_dist.rs @@ -35,7 +35,7 @@ impl Cylinder { pub fn from_components( pos: Vec3, scale: Option, - collider: Option, + collider: Option<&crate::comp::Collider>, char_state: Option<&crate::comp::CharacterState>, ) -> Self { let scale = scale.map_or(1.0, |s| s.0); diff --git a/common/sys/src/phys.rs b/common/sys/src/phys.rs index 95b6944ff4..9a6dd22748 100644 --- a/common/sys/src/phys.rs +++ b/common/sys/src/phys.rs @@ -108,7 +108,7 @@ pub struct PhysicsData<'a> { impl<'a> PhysicsData<'a> { /// Add/reset physics state components fn reset(&mut self) { - span!(guard, "Add/reset physics state components"); + span!(_guard, "Add/reset physics state components"); for (entity, _, _, _, _) in ( &self.read.entities, &self.read.colliders, @@ -124,11 +124,10 @@ impl<'a> PhysicsData<'a> { .entry(entity) .map(|e| e.or_insert_with(Default::default)); } - drop(guard); } fn maintain_pushback_cache(&mut self) { - span!(guard, "Maintain pushback cache"); + span!(_guard, "Maintain pushback cache"); //Add PreviousPhysCache for all relevant entities for entity in ( &self.read.entities, @@ -188,11 +187,10 @@ impl<'a> PhysicsData<'a> { phys_cache.scale = scale; phys_cache.scaled_radius = flat_radius; } - drop(guard); } fn apply_pushback(&mut self, job: &mut Job) { - span!(guard, "Apply pushback"); + span!(_guard, "Apply pushback"); job.cpu_stats.measure(ParMode::Rayon); let PhysicsData { ref read, @@ -368,7 +366,6 @@ impl<'a> PhysicsData<'a> { write.physics_metrics.entity_entity_collision_checks = metrics.entity_entity_collision_checks; write.physics_metrics.entity_entity_collisions = metrics.entity_entity_collisions; - drop(guard); } fn handle_movement_and_terrain(&mut self, job: &mut Job) { @@ -465,7 +462,7 @@ impl<'a> PhysicsData<'a> { _previous_cache, _, )| { - // defer the writes of positions and velocities to allow an inner loop over + // Defer the writes of positions and velocities to allow an inner loop over // terrain-like entities let mut vel = *vel; let old_vel = vel; @@ -525,7 +522,7 @@ impl<'a> PhysicsData<'a> { let mut cpos = *pos; let cylinder = (radius, z_min, z_max); - cylinder_voxel_collision( + box_voxel_collision( cylinder, &*read.terrain, entity, @@ -554,7 +551,7 @@ impl<'a> PhysicsData<'a> { let cylinder = (radius, z_min, z_max); let mut cpos = *pos; - cylinder_voxel_collision( + box_voxel_collision( cylinder, &*read.terrain, entity, @@ -668,99 +665,99 @@ impl<'a> PhysicsData<'a> { continue; } - if let Collider::Voxel { id } = collider_other { - // use bounding cylinder regardless of our collider - // TODO: extract point-terrain collision above to its own function - let radius = collider.get_radius(); - let (z_min, z_max) = collider.get_z_limits(1.0); + let voxel_id = if let Collider::Voxel { id } = collider_other { + id + } else { + continue; + }; + // use bounding cylinder regardless of our collider + // TODO: extract point-terrain collision above to its own function + let radius = collider.get_radius(); + let (z_min, z_max) = collider.get_z_limits(1.0); - let radius = radius.min(0.45) * scale; - let z_min = z_min * scale; - let z_max = z_max.clamped(1.2, 1.95) * scale; + let radius = radius.min(0.45) * scale; + let z_min = z_min * scale; + let z_max = z_max.clamped(1.2, 1.95) * scale; - if let Some(voxel_collider) = - VOXEL_COLLIDER_MANIFEST.read().colliders.get(&*id) - { - let mut physics_state_delta = physics_state.clone(); - // deliberately don't use scale yet here, because the 11.0/0.8 - // thing is in the comp::Scale for visual reasons - let mut cpos = *pos; - let wpos = cpos.0; + if let Some(voxel_collider) = + VOXEL_COLLIDER_MANIFEST.read().colliders.get(&*voxel_id) + { + let mut physics_state_delta = physics_state.clone(); + // deliberately don't use scale yet here, because the 11.0/0.8 + // thing is in the comp::Scale for visual reasons + let mut cpos = *pos; + let wpos = cpos.0; - // TODO: Cache the matrices here to avoid recomputing + // TODO: Cache the matrices here to avoid recomputing - let transform_from = - Mat4::::translation_3d(pos_other.0 - wpos) - * Mat4::from(ori_other.0) - * Mat4::::translation_3d(voxel_collider.translation); - let transform_to = transform_from.inverted(); - let ori_from = Mat4::from(ori_other.0); - let ori_to = ori_from.inverted(); + let transform_from = Mat4::::translation_3d(pos_other.0 - wpos) + * Mat4::from(ori_other.to_quat()) + * Mat4::::translation_3d(voxel_collider.translation); + let transform_to = transform_from.inverted(); + let ori_from = Mat4::from(ori_other.to_quat()); + let ori_to = ori_from.inverted(); - // The velocity of the collider, taking into account orientation. - let wpos_rel = (Mat4::::translation_3d(pos_other.0) - * Mat4::from(ori_other.0) - * Mat4::::translation_3d(voxel_collider.translation)) - .inverted() - .mul_point(wpos); - let wpos_last = (Mat4::::translation_3d(pos_other.0) - * Mat4::from(previous_cache_other.ori) - * Mat4::::translation_3d(voxel_collider.translation)) - .mul_point(wpos_rel); - let vel_other = vel_other.0 + (wpos - wpos_last) / read.dt.0; + // The velocity of the collider, taking into account orientation. + let wpos_rel = (Mat4::::translation_3d(pos_other.0) + * Mat4::from(ori_other.to_quat()) + * Mat4::::translation_3d(voxel_collider.translation)) + .inverted() + .mul_point(wpos); + let wpos_last = (Mat4::::translation_3d(pos_other.0) + * Mat4::from(previous_cache_other.ori) + * Mat4::::translation_3d(voxel_collider.translation)) + .mul_point(wpos_rel); + let vel_other = vel_other.0 + (wpos - wpos_last) / read.dt.0; - cpos.0 = transform_to.mul_point(Vec3::zero()); - vel.0 = ori_to.mul_direction(vel.0 - vel_other); - let cylinder = (radius, z_min, z_max); - cylinder_voxel_collision( - cylinder, - &voxel_collider.dyna, - entity, - &mut cpos, - transform_to.mul_point(tgt_pos - wpos), - &mut vel, - &mut physics_state_delta, - ori_to.mul_direction(vel_other), - &read.dt, - was_on_ground, - block_snap, - climbing, - |entity, vel| { - land_on_grounds - .push((entity, Vel(ori_from.mul_direction(vel.0)))) - }, - ); + cpos.0 = transform_to.mul_point(Vec3::zero()); + vel.0 = ori_to.mul_direction(vel.0 - vel_other); + let cylinder = (radius, z_min, z_max); + box_voxel_collision( + cylinder, + &voxel_collider.dyna, + entity, + &mut cpos, + transform_to.mul_point(tgt_pos - wpos), + &mut vel, + &mut physics_state_delta, + ori_to.mul_direction(vel_other), + &read.dt, + was_on_ground, + block_snap, + climbing, + |entity, vel| { + land_on_grounds + .push((entity, Vel(ori_from.mul_direction(vel.0)))) + }, + ); - cpos.0 = transform_from.mul_point(cpos.0) + wpos; - vel.0 = ori_from.mul_direction(vel.0) + vel_other; - tgt_pos = cpos.0; + cpos.0 = transform_from.mul_point(cpos.0) + wpos; + vel.0 = ori_from.mul_direction(vel.0) + vel_other; + tgt_pos = cpos.0; - // union in the state updates, so that the state isn't just based on - // the most recent terrain that collision was attempted with - if physics_state_delta.on_ground { - physics_state.ground_vel = vel_other; - } - physics_state.on_ground |= physics_state_delta.on_ground; - physics_state.on_ceiling |= physics_state_delta.on_ceiling; - physics_state.on_wall = physics_state.on_wall.or_else(|| { - physics_state_delta - .on_wall - .map(|dir| ori_from.mul_direction(dir)) - }); - physics_state - .touch_entities - .append(&mut physics_state_delta.touch_entities); - physics_state.in_liquid = match ( - physics_state.in_liquid, - physics_state_delta.in_liquid, - ) { + // union in the state updates, so that the state isn't just based on + // the most recent terrain that collision was attempted with + if physics_state_delta.on_ground { + physics_state.ground_vel = vel_other; + } + physics_state.on_ground |= physics_state_delta.on_ground; + physics_state.on_ceiling |= physics_state_delta.on_ceiling; + physics_state.on_wall = physics_state.on_wall.or_else(|| { + physics_state_delta + .on_wall + .map(|dir| ori_from.mul_direction(dir)) + }); + physics_state + .touch_entities + .append(&mut physics_state_delta.touch_entities); + physics_state.in_liquid = + match (physics_state.in_liquid, physics_state_delta.in_liquid) { // this match computes `x <|> y <|> liftA2 max x y` (Some(x), Some(y)) => Some(x.max(y)), (x @ Some(_), _) => x, (_, y @ Some(_)) => y, _ => None, }; - } } } @@ -804,7 +801,7 @@ impl<'a> PhysicsData<'a> { for (ori, previous_phys_cache) in (&write.orientations, &mut write.previous_phys_cache).join() { - previous_phys_cache.ori = ori.0; + previous_phys_cache.ori = ori.to_quat(); } let mut event_emitter = read.event_bus.emitter(); @@ -845,7 +842,7 @@ impl<'a> System<'a> for Sys { } #[allow(clippy::too_many_arguments)] -fn cylinder_voxel_collision<'a, T: BaseVol + ReadVol>( +fn box_voxel_collision<'a, T: BaseVol + ReadVol>( cylinder: (f32, f32, f32), terrain: &'a T, entity: Entity, diff --git a/server/src/events/inventory_manip.rs b/server/src/events/inventory_manip.rs index 4005c3131b..c03290d370 100644 --- a/server/src/events/inventory_manip.rs +++ b/server/src/events/inventory_manip.rs @@ -69,7 +69,7 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Inv find_dist::Cylinder::from_components( p.0, scales.get(entity).copied(), - colliders.get(entity).cloned(), + colliders.get(entity), char_states.get(entity), ) }) diff --git a/server/src/sys/entity_sync.rs b/server/src/sys/entity_sync.rs index c08e2874ea..600fea43fe 100644 --- a/server/src/sys/entity_sync.rs +++ b/server/src/sys/entity_sync.rs @@ -250,7 +250,7 @@ impl<'a> System<'a> for Sys { let mut comp_sync_package = CompSyncPackage::new(); let mut throttle = true; - // extrapolation depends on receiving several frames indicating that something + // Extrapolation depends on receiving several frames indicating that something // has stopped in order for the extrapolated value to have // stopped const SEND_UNCHANGED_PHYSICS_DATA: bool = true; diff --git a/voxygen/src/session.rs b/voxygen/src/session.rs index 8bcbffd4bf..791db45343 100644 --- a/voxygen/src/session.rs +++ b/voxygen/src/session.rs @@ -1562,7 +1562,7 @@ fn under_cursor( let player_cylinder = Cylinder::from_components( player_pos, scales.get(player_entity).copied(), - colliders.get(player_entity).cloned(), + colliders.get(player_entity), char_states.get(player_entity), ); let terrain = client.state().terrain(); @@ -1643,7 +1643,7 @@ fn under_cursor( let target_cylinder = Cylinder::from_components( p, scales.get(*e).copied(), - colliders.get(*e).cloned(), + colliders.get(*e), char_states.get(*e), ); @@ -1706,7 +1706,7 @@ fn select_interactable( let player_cylinder = Cylinder::from_components( player_pos, scales.get(player_entity).copied(), - colliders.get(player_entity).cloned(), + colliders.get(player_entity), char_states.get(player_entity), ); @@ -1720,7 +1720,7 @@ fn select_interactable( .join() .filter(|(e, _, _, _, _)| *e != player_entity) .map(|(e, p, s, c, cs)| { - let cylinder = Cylinder::from_components(p.0, s.copied(), c.cloned(), cs); + let cylinder = Cylinder::from_components(p.0, s.copied(), c, cs); (e, cylinder) }) // Roughly filter out entities farther than interaction distance