diff options
author | Robert Adams | 2012-11-27 05:37:06 -0800 |
---|---|---|
committer | Robert Adams | 2012-11-27 10:03:49 -0800 |
commit | 68fe7dff20fb38480a1c760f1347dafac43899c5 (patch) | |
tree | 14aeaf2d342f8a7cd6cf4c4a906ec1342e4ae43c /OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs | |
parent | BulletSim: implementation of vertical attraction motor. (diff) | |
download | opensim-SC_OLD-68fe7dff20fb38480a1c760f1347dafac43899c5.zip opensim-SC_OLD-68fe7dff20fb38480a1c760f1347dafac43899c5.tar.gz opensim-SC_OLD-68fe7dff20fb38480a1c760f1347dafac43899c5.tar.bz2 opensim-SC_OLD-68fe7dff20fb38480a1c760f1347dafac43899c5.tar.xz |
BulletSim: reorganize angular movement routine into separate subroutines enabling external calibration routines and unit testing.
Diffstat (limited to 'OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs')
-rw-r--r-- | OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs | 209 |
1 files changed, 114 insertions, 95 deletions
diff --git a/OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs b/OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs index d94abf4..eb4d06a 100644 --- a/OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs +++ b/OpenSim/Region/Physics/BulletSPlugin/BSDynamics.cs | |||
@@ -317,7 +317,7 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
317 | m_VhoverEfficiency = 0; | 317 | m_VhoverEfficiency = 0; |
318 | m_VhoverTimescale = 0; | 318 | m_VhoverTimescale = 0; |
319 | m_VehicleBuoyancy = 0; | 319 | m_VehicleBuoyancy = 0; |
320 | 320 | ||
321 | m_linearDeflectionEfficiency = 1; | 321 | m_linearDeflectionEfficiency = 1; |
322 | m_linearDeflectionTimescale = 1; | 322 | m_linearDeflectionTimescale = 1; |
323 | 323 | ||
@@ -366,8 +366,8 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
366 | m_bankingMix = 1; | 366 | m_bankingMix = 1; |
367 | 367 | ||
368 | m_referenceFrame = Quaternion.Identity; | 368 | m_referenceFrame = Quaternion.Identity; |
369 | m_flags &= ~(VehicleFlag.HOVER_WATER_ONLY | 369 | m_flags &= ~(VehicleFlag.HOVER_WATER_ONLY |
370 | | VehicleFlag.HOVER_TERRAIN_ONLY | 370 | | VehicleFlag.HOVER_TERRAIN_ONLY |
371 | | VehicleFlag.HOVER_GLOBAL_HEIGHT | 371 | | VehicleFlag.HOVER_GLOBAL_HEIGHT |
372 | | VehicleFlag.HOVER_UP_ONLY); | 372 | | VehicleFlag.HOVER_UP_ONLY); |
373 | m_flags |= (VehicleFlag.NO_DEFLECTION_UP | 373 | m_flags |= (VehicleFlag.NO_DEFLECTION_UP |
@@ -575,7 +575,7 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
575 | Vector3 localInertia = new Vector3(m_vehicleMass, m_vehicleMass, m_vehicleMass); | 575 | Vector3 localInertia = new Vector3(m_vehicleMass, m_vehicleMass, m_vehicleMass); |
576 | BulletSimAPI.SetMassProps2(Prim.PhysBody.ptr, m_vehicleMass, localInertia); | 576 | BulletSimAPI.SetMassProps2(Prim.PhysBody.ptr, m_vehicleMass, localInertia); |
577 | 577 | ||
578 | VDetailLog("{0},BSDynamics.Refresh,frict={1},inert={2},aDamp={3}", | 578 | VDetailLog("{0},BSDynamics.Refresh,frict={1},inert={2},aDamp={3}", |
579 | Prim.LocalID, friction, localInertia, angularDamping); | 579 | Prim.LocalID, friction, localInertia, angularDamping); |
580 | } | 580 | } |
581 | } | 581 | } |
@@ -759,13 +759,13 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
759 | // (http://wiki.secondlife.com/wiki/Category:LSL_Vehicle), the downForce | 759 | // (http://wiki.secondlife.com/wiki/Category:LSL_Vehicle), the downForce |
760 | // has a decay factor. This says this force should | 760 | // has a decay factor. This says this force should |
761 | // be computed with a motor. | 761 | // be computed with a motor. |
762 | VDetailLog("{0},MoveLinear,limitMotorUp,distAbove={1},downForce={2}", | 762 | VDetailLog("{0},MoveLinear,limitMotorUp,distAbove={1},downForce={2}", |
763 | Prim.LocalID, distanceAboveGround, limitMotorUpContribution); | 763 | Prim.LocalID, distanceAboveGround, limitMotorUpContribution); |
764 | } | 764 | } |
765 | 765 | ||
766 | // ================================================================== | 766 | // ================================================================== |
767 | Vector3 newVelocity = linearMotorContribution | 767 | Vector3 newVelocity = linearMotorContribution |
768 | + terrainHeightContribution | 768 | + terrainHeightContribution |
769 | + hoverContribution | 769 | + hoverContribution |
770 | + limitMotorUpContribution; | 770 | + limitMotorUpContribution; |
771 | 771 | ||
@@ -801,7 +801,7 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
801 | } | 801 | } |
802 | 802 | ||
803 | VDetailLog("{0},MoveLinear,done,lmDir={1},lmVel={2},newVel={3},primVel={4},totalDown={5}", | 803 | VDetailLog("{0},MoveLinear,done,lmDir={1},lmVel={2},newVel={3},primVel={4},totalDown={5}", |
804 | Prim.LocalID, m_linearMotorDirection, m_lastLinearVelocityVector, | 804 | Prim.LocalID, m_linearMotorDirection, m_lastLinearVelocityVector, |
805 | newVelocity, Prim.Velocity, totalDownForce); | 805 | newVelocity, Prim.Velocity, totalDownForce); |
806 | 806 | ||
807 | } // end MoveLinear() | 807 | } // end MoveLinear() |
@@ -850,8 +850,84 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
850 | VDetailLog("{0},MoveAngular,noDeflectionUp,angularMotorContrib={1}", Prim.LocalID, angularMotorContribution); | 850 | VDetailLog("{0},MoveAngular,noDeflectionUp,angularMotorContrib={1}", Prim.LocalID, angularMotorContribution); |
851 | } | 851 | } |
852 | 852 | ||
853 | Vector3 verticalAttractionContribution = ComputeAngularVerticalAttraction(pTimestep); | ||
854 | |||
855 | Vector3 deflectionContribution = ComputeAngularDeflection(pTimestep); | ||
856 | |||
857 | Vector3 bankingContribution = ComputeAngularBanking(pTimestep); | ||
858 | |||
859 | // ================================================================== | ||
860 | m_lastVertAttractor = verticalAttractionContribution; | ||
861 | |||
862 | // Sum velocities | ||
863 | m_lastAngularVelocity = angularMotorContribution | ||
864 | + verticalAttractionContribution | ||
865 | + bankingContribution | ||
866 | + deflectionContribution; | ||
867 | |||
868 | // ================================================================== | ||
869 | //Offset section | ||
870 | if (m_linearMotorOffset != Vector3.Zero) | ||
871 | { | ||
872 | //Offset of linear velocity doesn't change the linear velocity, | ||
873 | // but causes a torque to be applied, for example... | ||
874 | // | ||
875 | // IIIII >>> IIIII | ||
876 | // IIIII >>> IIIII | ||
877 | // IIIII >>> IIIII | ||
878 | // ^ | ||
879 | // | Applying a force at the arrow will cause the object to move forward, but also rotate | ||
880 | // | ||
881 | // | ||
882 | // The torque created is the linear velocity crossed with the offset | ||
883 | |||
884 | // TODO: this computation should be in the linear section | ||
885 | // because that is where we know the impulse being applied. | ||
886 | Vector3 torqueFromOffset = Vector3.Zero; | ||
887 | // torqueFromOffset = Vector3.Cross(m_linearMotorOffset, appliedImpulse); | ||
888 | if (float.IsNaN(torqueFromOffset.X)) | ||
889 | torqueFromOffset.X = 0; | ||
890 | if (float.IsNaN(torqueFromOffset.Y)) | ||
891 | torqueFromOffset.Y = 0; | ||
892 | if (float.IsNaN(torqueFromOffset.Z)) | ||
893 | torqueFromOffset.Z = 0; | ||
894 | torqueFromOffset *= m_vehicleMass; | ||
895 | Prim.ApplyTorqueImpulse(torqueFromOffset, true); | ||
896 | VDetailLog("{0},BSDynamic.MoveAngular,motorOffset,applyTorqueImpulse={1}", Prim.LocalID, torqueFromOffset); | ||
897 | } | ||
898 | |||
853 | // ================================================================== | 899 | // ================================================================== |
854 | Vector3 verticalAttractionContribution = Vector3.Zero; | 900 | if (m_lastAngularVelocity.ApproxEquals(Vector3.Zero, 0.01f)) |
901 | { | ||
902 | m_lastAngularVelocity = Vector3.Zero; // Reduce small value to zero. | ||
903 | Prim.ZeroAngularMotion(true); | ||
904 | VDetailLog("{0},MoveAngular,zeroAngularMotion,lastAngular={1}", Prim.LocalID, m_lastAngularVelocity); | ||
905 | } | ||
906 | else | ||
907 | { | ||
908 | // Apply to the body. | ||
909 | // The above calculates the absolute angular velocity needed. Angular velocity is massless. | ||
910 | // Since we are stuffing the angular velocity directly into the object, the computed | ||
911 | // velocity needs to be scaled by the timestep. | ||
912 | // Also remove any motion that is on the object so added motion is only from vehicle. | ||
913 | Vector3 applyAngularForce = ((m_lastAngularVelocity * pTimestep) | ||
914 | - Prim.ForceRotationalVelocity); | ||
915 | // Unscale the force by the angular factor so it overwhelmes the Bullet additions. | ||
916 | Prim.ForceRotationalVelocity = applyAngularForce; | ||
917 | |||
918 | VDetailLog("{0},MoveAngular,done,angMotor={1},vertAttr={2},bank={3},deflect={4},newAngForce={5},lastAngular={6}", | ||
919 | Prim.LocalID, | ||
920 | angularMotorContribution, verticalAttractionContribution, | ||
921 | bankingContribution, deflectionContribution, | ||
922 | applyAngularForce, m_lastAngularVelocity | ||
923 | ); | ||
924 | } | ||
925 | } | ||
926 | |||
927 | public Vector3 ComputeAngularVerticalAttraction(float pTimestep) | ||
928 | { | ||
929 | Vector3 ret = Vector3.Zero; | ||
930 | |||
855 | // If vertical attaction timescale is reasonable and we applied an angular force last time... | 931 | // If vertical attaction timescale is reasonable and we applied an angular force last time... |
856 | if (m_verticalAttractionTimescale < 500) | 932 | if (m_verticalAttractionTimescale < 500) |
857 | { | 933 | { |
@@ -859,7 +935,7 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
859 | verticalError.Normalize(); | 935 | verticalError.Normalize(); |
860 | m_verticalAttractionMotor.SetCurrent(verticalError); | 936 | m_verticalAttractionMotor.SetCurrent(verticalError); |
861 | m_verticalAttractionMotor.SetTarget(Vector3.UnitZ); | 937 | m_verticalAttractionMotor.SetTarget(Vector3.UnitZ); |
862 | verticalAttractionContribution = m_verticalAttractionMotor.Step(pTimestep); | 938 | ret = m_verticalAttractionMotor.Step(pTimestep); |
863 | /* | 939 | /* |
864 | // Take a vector pointing up and convert it from world to vehicle relative coords. | 940 | // Take a vector pointing up and convert it from world to vehicle relative coords. |
865 | Vector3 verticalError = Vector3.UnitZ * Prim.ForceOrientation; | 941 | Vector3 verticalError = Vector3.UnitZ * Prim.ForceOrientation; |
@@ -895,15 +971,19 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
895 | verticalAttractionContribution *= (m_verticalAttractionEfficiency * m_verticalAttractionEfficiency); | 971 | verticalAttractionContribution *= (m_verticalAttractionEfficiency * m_verticalAttractionEfficiency); |
896 | 972 | ||
897 | VDetailLog("{0},MoveAngular,verticalAttraction,,verticalError={1},unscaled={2},preEff={3},eff={4},effSq={5},vertAttr={6}", | 973 | VDetailLog("{0},MoveAngular,verticalAttraction,,verticalError={1},unscaled={2},preEff={3},eff={4},effSq={5},vertAttr={6}", |
898 | Prim.LocalID, verticalError, unscaledContrib, preEfficiencyContrib, | 974 | Prim.LocalID, verticalError, unscaledContrib, preEfficiencyContrib, |
899 | m_verticalAttractionEfficiency, efficencySquared, | 975 | m_verticalAttractionEfficiency, efficencySquared, |
900 | verticalAttractionContribution); | 976 | verticalAttractionContribution); |
901 | */ | 977 | */ |
902 | 978 | ||
903 | } | 979 | } |
980 | return ret; | ||
981 | } | ||
982 | |||
983 | public Vector3 ComputeAngularDeflection(float pTimestep) | ||
984 | { | ||
985 | Vector3 ret = Vector3.Zero; | ||
904 | 986 | ||
905 | // ================================================================== | ||
906 | Vector3 deflectionContribution = Vector3.Zero; | ||
907 | if (m_angularDeflectionEfficiency != 0) | 987 | if (m_angularDeflectionEfficiency != 0) |
908 | { | 988 | { |
909 | // Compute a scaled vector that points in the preferred axis (X direction) | 989 | // Compute a scaled vector that points in the preferred axis (X direction) |
@@ -914,24 +994,28 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
914 | Vector3 preferredAxisOfMotion = scaledDefaultDirection * Quaternion.Add(Prim.ForceOrientation, m_referenceFrame); | 994 | Vector3 preferredAxisOfMotion = scaledDefaultDirection * Quaternion.Add(Prim.ForceOrientation, m_referenceFrame); |
915 | 995 | ||
916 | // Scale by efficiency and timescale | 996 | // Scale by efficiency and timescale |
917 | deflectionContribution = (preferredAxisOfMotion * (m_angularDeflectionEfficiency) / m_angularDeflectionTimescale) * pTimestep; | 997 | ret = (preferredAxisOfMotion * (m_angularDeflectionEfficiency) / m_angularDeflectionTimescale) * pTimestep; |
998 | |||
999 | VDetailLog("{0},MoveAngular,Deflection,perfAxis={1},deflection={2}", Prim.LocalID, preferredAxisOfMotion, ret); | ||
918 | 1000 | ||
919 | VDetailLog("{0},MoveAngular,Deflection,perfAxis={1},deflection={2}", | ||
920 | Prim.LocalID, preferredAxisOfMotion, deflectionContribution); | ||
921 | // This deflection computation is not correct. | 1001 | // This deflection computation is not correct. |
922 | deflectionContribution = Vector3.Zero; | 1002 | ret = Vector3.Zero; |
923 | } | 1003 | } |
1004 | return ret; | ||
1005 | } | ||
1006 | |||
1007 | public Vector3 ComputeAngularBanking(float pTimestep) | ||
1008 | { | ||
1009 | Vector3 ret = Vector3.Zero; | ||
924 | 1010 | ||
925 | // ================================================================== | ||
926 | Vector3 bankingContribution = Vector3.Zero; | ||
927 | if (m_bankingEfficiency != 0) | 1011 | if (m_bankingEfficiency != 0) |
928 | { | 1012 | { |
929 | Vector3 dir = Vector3.One * Prim.ForceOrientation; | 1013 | Vector3 dir = Vector3.One * Prim.ForceOrientation; |
930 | float mult = (m_bankingMix*m_bankingMix)*-1*(m_bankingMix < 0 ? -1 : 1); | 1014 | float mult = (m_bankingMix * m_bankingMix) * -1 * (m_bankingMix < 0 ? -1 : 1); |
931 | //Changes which way it banks in and out of turns | 1015 | //Changes which way it banks in and out of turns |
932 | 1016 | ||
933 | //Use the square of the efficiency, as it looks much more how SL banking works | 1017 | //Use the square of the efficiency, as it looks much more how SL banking works |
934 | float effSquared = (m_bankingEfficiency*m_bankingEfficiency); | 1018 | float effSquared = (m_bankingEfficiency * m_bankingEfficiency); |
935 | if (m_bankingEfficiency < 0) | 1019 | if (m_bankingEfficiency < 0) |
936 | effSquared *= -1; //Keep the negative! | 1020 | effSquared *= -1; //Keep the negative! |
937 | 1021 | ||
@@ -953,99 +1037,34 @@ namespace OpenSim.Region.Physics.BulletSPlugin | |||
953 | } | 1037 | } |
954 | else | 1038 | else |
955 | { | 1039 | { |
956 | bankingContribution.Z += (effSquared * (mult * mix)) * (m_angularMotorVelocity.X) * 4; | 1040 | ret.Z += (effSquared * (mult * mix)) * (m_angularMotorVelocity.X) * 4; |
957 | } | 1041 | } |
958 | 1042 | ||
959 | //If they are colliding, we probably shouldn't shove the prim around... probably | 1043 | //If they are colliding, we probably shouldn't shove the prim around... probably |
960 | if (!Prim.IsColliding && Math.Abs(m_angularMotorVelocity.X) > mix) | 1044 | if (!Prim.IsColliding && Math.Abs(m_angularMotorVelocity.X) > mix) |
961 | { | 1045 | { |
962 | float angVelZ = m_angularMotorVelocity.X*-1; | 1046 | float angVelZ = m_angularMotorVelocity.X * -1; |
963 | /*if(angVelZ > mix) | 1047 | /*if(angVelZ > mix) |
964 | angVelZ = mix; | 1048 | angVelZ = mix; |
965 | else if(angVelZ < -mix) | 1049 | else if(angVelZ < -mix) |
966 | angVelZ = -mix;*/ | 1050 | angVelZ = -mix;*/ |
967 | //This controls how fast and how far the banking occurs | 1051 | //This controls how fast and how far the banking occurs |
968 | Vector3 bankingRot = new Vector3(angVelZ*(effSquared*mult), 0, 0); | 1052 | Vector3 bankingRot = new Vector3(angVelZ * (effSquared * mult), 0, 0); |
969 | if (bankingRot.X > 3) | 1053 | if (bankingRot.X > 3) |
970 | bankingRot.X = 3; | 1054 | bankingRot.X = 3; |
971 | else if (bankingRot.X < -3) | 1055 | else if (bankingRot.X < -3) |
972 | bankingRot.X = -3; | 1056 | bankingRot.X = -3; |
973 | bankingRot *= Prim.ForceOrientation; | 1057 | bankingRot *= Prim.ForceOrientation; |
974 | bankingContribution += bankingRot; | 1058 | ret += bankingRot; |
975 | } | 1059 | } |
976 | m_angularMotorVelocity.X *= m_bankingEfficiency == 1 ? 0.0f : 1 - m_bankingEfficiency; | 1060 | m_angularMotorVelocity.X *= m_bankingEfficiency == 1 ? 0.0f : 1 - m_bankingEfficiency; |
977 | VDetailLog("{0},MoveAngular,Banking,bEff={1},angMotVel={2},effSq={3},mult={4},mix={5},banking={6}", | 1061 | VDetailLog("{0},MoveAngular,Banking,bEff={1},angMotVel={2},effSq={3},mult={4},mix={5},banking={6}", |
978 | Prim.LocalID, m_bankingEfficiency, m_angularMotorVelocity, effSquared, mult, mix, bankingContribution); | 1062 | Prim.LocalID, m_bankingEfficiency, m_angularMotorVelocity, effSquared, mult, mix, ret); |
979 | } | ||
980 | |||
981 | // ================================================================== | ||
982 | m_lastVertAttractor = verticalAttractionContribution; | ||
983 | |||
984 | // Sum velocities | ||
985 | m_lastAngularVelocity = angularMotorContribution | ||
986 | + verticalAttractionContribution | ||
987 | + bankingContribution | ||
988 | + deflectionContribution; | ||
989 | |||
990 | // ================================================================== | ||
991 | //Offset section | ||
992 | if (m_linearMotorOffset != Vector3.Zero) | ||
993 | { | ||
994 | //Offset of linear velocity doesn't change the linear velocity, | ||
995 | // but causes a torque to be applied, for example... | ||
996 | // | ||
997 | // IIIII >>> IIIII | ||
998 | // IIIII >>> IIIII | ||
999 | // IIIII >>> IIIII | ||
1000 | // ^ | ||
1001 | // | Applying a force at the arrow will cause the object to move forward, but also rotate | ||
1002 | // | ||
1003 | // | ||
1004 | // The torque created is the linear velocity crossed with the offset | ||
1005 | |||
1006 | // TODO: this computation should be in the linear section | ||
1007 | // because that is where we know the impulse being applied. | ||
1008 | Vector3 torqueFromOffset = Vector3.Zero; | ||
1009 | // torqueFromOffset = Vector3.Cross(m_linearMotorOffset, appliedImpulse); | ||
1010 | if (float.IsNaN(torqueFromOffset.X)) | ||
1011 | torqueFromOffset.X = 0; | ||
1012 | if (float.IsNaN(torqueFromOffset.Y)) | ||
1013 | torqueFromOffset.Y = 0; | ||
1014 | if (float.IsNaN(torqueFromOffset.Z)) | ||
1015 | torqueFromOffset.Z = 0; | ||
1016 | torqueFromOffset *= m_vehicleMass; | ||
1017 | Prim.ApplyTorqueImpulse(torqueFromOffset, true); | ||
1018 | VDetailLog("{0},BSDynamic.MoveAngular,motorOffset,applyTorqueImpulse={1}", Prim.LocalID, torqueFromOffset); | ||
1019 | } | ||
1020 | |||
1021 | // ================================================================== | ||
1022 | if (m_lastAngularVelocity.ApproxEquals(Vector3.Zero, 0.01f)) | ||
1023 | { | ||
1024 | m_lastAngularVelocity = Vector3.Zero; // Reduce small value to zero. | ||
1025 | Prim.ZeroAngularMotion(true); | ||
1026 | VDetailLog("{0},MoveAngular,zeroAngularMotion,lastAngular={1}", Prim.LocalID, m_lastAngularVelocity); | ||
1027 | } | ||
1028 | else | ||
1029 | { | ||
1030 | // Apply to the body. | ||
1031 | // The above calculates the absolute angular velocity needed. Angular velocity is massless. | ||
1032 | // Since we are stuffing the angular velocity directly into the object, the computed | ||
1033 | // velocity needs to be scaled by the timestep. | ||
1034 | // Also remove any motion that is on the object so added motion is only from vehicle. | ||
1035 | Vector3 applyAngularForce = ((m_lastAngularVelocity * pTimestep) | ||
1036 | - Prim.ForceRotationalVelocity); | ||
1037 | // Unscale the force by the angular factor so it overwhelmes the Bullet additions. | ||
1038 | Prim.ForceRotationalVelocity = applyAngularForce; | ||
1039 | |||
1040 | VDetailLog("{0},MoveAngular,done,angMotor={1},vertAttr={2},bank={3},deflect={4},newAngForce={5},lastAngular={6}", | ||
1041 | Prim.LocalID, | ||
1042 | angularMotorContribution, verticalAttractionContribution, | ||
1043 | bankingContribution, deflectionContribution, | ||
1044 | applyAngularForce, m_lastAngularVelocity | ||
1045 | ); | ||
1046 | } | 1063 | } |
1064 | return ret; | ||
1047 | } | 1065 | } |
1048 | 1066 | ||
1067 | |||
1049 | // This is from previous instantiations of XXXDynamics.cs. | 1068 | // This is from previous instantiations of XXXDynamics.cs. |
1050 | // Applies roll reference frame. | 1069 | // Applies roll reference frame. |
1051 | // TODO: is this the right way to separate the code to do this operation? | 1070 | // TODO: is this the right way to separate the code to do this operation? |