Address PR comments

include/openspace/interaction/joystickcamerastates.h

@@ -54,7 +54,7 @@ public:
     };
 
     enum class JoystickType {
-        JoystickLike = 0,
+        JoystickLike,
         TriggerLike
     };
 
@@ -88,14 +88,14 @@ public:
     void updateStateFromInput(
         const JoystickInputStates& joystickInputStates, double deltaTime);
 
-    void setAxisMapping(const std::string& joystickName, int axis, AxisType mapping,
+    void setAxisMapping(std::string joystickName, int axis, AxisType mapping,
         AxisInvert shouldInvert = AxisInvert::No,
         JoystickType joystickType = JoystickType::JoystickLike,
         bool isSticky = false, double sensitivity = 0.0
     );
 
-    void setAxisMappingProperty(const std::string& joystickName, int axis,
-        const std::string& propertyUri, float min = 0.f, float max = 1.f,
+    void setAxisMappingProperty(std::string joystickName, int axis,
+        std::string propertyUri, float min = 0.f, float max = 1.f,
         AxisInvert shouldInvert = AxisInvert::No, bool isRemote = true
     );
 
@@ -119,7 +119,6 @@ private:
         // be accessed much more often and thus have to be more efficient. And storing a few
         // extra AxisInformation that are not used will not matter that much; finding an axis
         // location in a potential map each frame, however, would
-
         std::array<AxisInformation, JoystickInputState::MaxAxes> axisMapping;
 
         // This array is used to store the old axis values from the previous frame,
@@ -127,9 +126,16 @@ private:
         std::array<float, JoystickInputState::MaxAxes> prevAxisValues;
 
         struct ButtonInformation {
+            // The script that is run when the button is activated
             std::string command;
+
+            // When is the button considered activated
             JoystickAction action;
+
+            // If the script should be syncronised to other remote sessions or not
             ButtonCommandRemote synchronization;
+
+            // Short documentation on what the script of this button does
             std::string documentation;
         };
 
@@ -140,8 +146,8 @@ private:
 
     // Find the item in _joystickCameraStates that corresponds to the given joystickName
     // return a pointer to the item, if not found then return nullptr
-    JoystickCameraState* getJoystickCameraState(const std::string& joystickName);
-    const JoystickCameraState* getJoystickCameraState(const std::string& joystickName) const;
+    JoystickCameraState* joystickCameraState(const std::string& joystickName);
+    const JoystickCameraState* joystickCameraState(const std::string& joystickName) const;
 
     // Ues getJoystickCameraState(name) to find the joystickCameraState that
     // corresponds to the given joystickName. If not found then add a new item if possible
@@ -204,9 +210,9 @@ inline std::string to_string(
 {
     using T = openspace::interaction::JoystickCameraStates::JoystickType;
     switch (value) {
-    case T::JoystickLike: return "JoystickLike";
-    case T::TriggerLike:  return "TriggerLike";
-    default:              return "";
+        case T::JoystickLike: return "JoystickLike";
+        case T::TriggerLike:  return "TriggerLike";
+        default:              return "";
     }
 }
 

include/openspace/navigation/navigationhandler.h

@@ -98,7 +98,7 @@ public:
     void mousePositionCallback(double x, double y);
     void mouseScrollWheelCallback(double pos);
 
-    void setJoystickAxisMapping(const std::string& joystickName,
+    void setJoystickAxisMapping(std::string joystickName,
         int axis, JoystickCameraStates::AxisType mapping,
         JoystickCameraStates::AxisInvert shouldInvert =
             JoystickCameraStates::AxisInvert::No,
@@ -107,8 +107,8 @@ public:
         bool isSticky = false, double sensitivity = 0.0
     );
 
-    void setJoystickAxisMappingProperty(const std::string& joystickName,
-        int axis, const std::string& propertyUri,
+    void setJoystickAxisMappingProperty(std::string joystickName,
+        int axis, std::string propertyUri,
         float min = 0.f, float max = 1.f,
         JoystickCameraStates::AxisInvert shouldInvert =
         JoystickCameraStates::AxisInvert::No, bool isRemote = true

src/interaction/joystickcamerastates.cpp

@@ -56,15 +56,14 @@ void JoystickCameraStates::updateStateFromInput(
             continue;
         }
 
-        JoystickCameraState* joystickCameraState =
-            getJoystickCameraState(joystickInputState.name);
+        JoystickCameraState* joystick = joystickCameraState(joystickInputState.name);
 
-        if (!joystickCameraState) {
+        if (!joystick) {
             continue;
         }
 
         for (int i = 0; i < JoystickInputState::MaxAxes; ++i) {
-            AxisInformation t = joystickCameraState->axisMapping[i];
+            AxisInformation t = joystick->axisMapping[i];
             if (t.type == AxisType::None) {
                 continue;
             }
@@ -73,13 +72,13 @@ void JoystickCameraStates::updateStateFromInput(
             float value = rawValue;
 
             if (t.isSticky) {
-                value = rawValue - joystickCameraState->prevAxisValues[i];
-                joystickCameraState->prevAxisValues[i] = rawValue;
+                value = rawValue - joystick->prevAxisValues[i];
+                joystick->prevAxisValues[i] = rawValue;
             }
 
             if ((t.joystickType == JoystickType::JoystickLike &&
-                 std::fabs(value) <= t.deadzone) ||
-                (t.joystickType == JoystickType::TriggerLike && value <= -1 + t.deadzone))
+                 std::abs(value) <= t.deadzone) ||
+                (t.joystickType == JoystickType::TriggerLike && value <= -1.f + t.deadzone))
             {
                 continue;
             }
@@ -151,8 +150,8 @@ void JoystickCameraStates::updateStateFromInput(
                     localRotation.second.y += value;
                     break;
                 case AxisType::Property:
-                    std::string script = "openspace.setPropertyValue(\"" +
-                        t.propertyUri + "\", " + std::to_string(value) + ");";
+                    std::string script = fmt::format("openspace.setPropertyValue('{}', {});",
+                        t.propertyUri, value);
 
                     global::scriptEngine->queueScript(
                         script,
@@ -163,7 +162,7 @@ void JoystickCameraStates::updateStateFromInput(
         }
 
         for (int i = 0; i < JoystickInputState::MaxButtons; ++i) {
-            auto itRange = joystickCameraState->buttonMapping.equal_range(i);
+            auto itRange = joystick->buttonMapping.equal_range(i);
             for (auto it = itRange.first; it != itRange.second; ++it) {
                 bool active = global::joystickInputStates->button(
                     joystickInputState.name,
@@ -217,7 +216,7 @@ void JoystickCameraStates::updateStateFromInput(
     }
 }
 
-void JoystickCameraStates::setAxisMapping(const std::string& joystickName,
+void JoystickCameraStates::setAxisMapping(std::string joystickName,
                                           int axis, AxisType mapping,
                                           AxisInvert shouldInvert,
                                           JoystickType joystickType,
@@ -241,9 +240,9 @@ void JoystickCameraStates::setAxisMapping(const std::string& joystickName,
         global::joystickInputStates->axis(joystickName, axis);
 }
 
-void JoystickCameraStates::setAxisMappingProperty(const std::string& joystickName,
+void JoystickCameraStates::setAxisMappingProperty(std::string joystickName,
                                                   int axis,
-                                                  const std::string& propertyUri,
+                                                  std::string propertyUri,
                                                   float min, float max,
                                                   AxisInvert shouldInvert,
                                                   bool isRemote)
@@ -270,13 +269,13 @@ JoystickCameraStates::AxisInformation JoystickCameraStates::axisMapping(
                                                           const std::string& joystickName,
                                                                         int axis) const
 {
-    const JoystickCameraState* joystickCameraState = getJoystickCameraState(joystickName);
-    if (!joystickCameraState) {
+    const JoystickCameraState* joystick = joystickCameraState(joystickName);
+    if (!joystick) {
         JoystickCameraStates::AxisInformation dummy;
         return dummy;
     }
 
-    return joystickCameraState->axisMapping[axis];
+    return joystick->axisMapping[axis];
 }
 
 void JoystickCameraStates::setDeadzone(const std::string& joystickName, int axis,
@@ -291,12 +290,12 @@ void JoystickCameraStates::setDeadzone(const std::string& joystickName, int axis
 }
 
 float JoystickCameraStates::deadzone(const std::string& joystickName, int axis) const {
-    const JoystickCameraState* joystickCameraState = getJoystickCameraState(joystickName);
-    if (!joystickCameraState) {
-        return 0.0f;
+    const JoystickCameraState* joystick = joystickCameraState(joystickName);
+    if (!joystick) {
+        return 0.f;
     }
 
-    return joystickCameraState->axisMapping[axis].deadzone;
+    return joystick->axisMapping[axis].deadzone;
 }
 
 void JoystickCameraStates::bindButtonCommand(const std::string& joystickName,
@@ -319,18 +318,18 @@ void JoystickCameraStates::bindButtonCommand(const std::string& joystickName,
 void JoystickCameraStates::clearButtonCommand(const std::string& joystickName,
                                               int button)
 {
-    JoystickCameraState* joystickCameraState = getJoystickCameraState(joystickName);
-    if (!joystickCameraState) {
+    JoystickCameraState* joystick = joystickCameraState(joystickName);
+    if (!joystick) {
         return;
     }
 
-    for (auto it = joystickCameraState->buttonMapping.begin();
-         it != joystickCameraState->buttonMapping.end(); )
+    for (auto it = joystick->buttonMapping.begin();
+         it != joystick->buttonMapping.end(); )
     {
         // If the current iterator is the button that we are looking for, delete it
         // (std::multimap::erase will return the iterator to the next element for us)
         if (it->first == button) {
-            it = joystickCameraState->buttonMapping.erase(it);
+            it = joystick->buttonMapping.erase(it);
         }
         else {
             ++it;
@@ -343,19 +342,19 @@ std::vector<std::string> JoystickCameraStates::buttonCommand(
                                                              int button) const
 {
     std::vector<std::string> result;
-    const JoystickCameraState* joystickCameraState = getJoystickCameraState(joystickName);
-    if (!joystickCameraState) {
+    const JoystickCameraState* joystick = joystickCameraState(joystickName);
+    if (!joystick) {
         return result;
     }
 
-    auto itRange = joystickCameraState->buttonMapping.equal_range(button);
+    auto itRange = joystick->buttonMapping.equal_range(button);
     for (auto it = itRange.first; it != itRange.second; ++it) {
         result.push_back(it->second.command);
     }
     return result;
 }
 
-JoystickCameraStates::JoystickCameraState* JoystickCameraStates::getJoystickCameraState(
+JoystickCameraStates::JoystickCameraState* JoystickCameraStates::joystickCameraState(
                                                           const std::string& joystickName)
 {
     for (JoystickCameraState& joystickCameraState : _joystickCameraStates) {
@@ -368,7 +367,7 @@ JoystickCameraStates::JoystickCameraState* JoystickCameraStates::getJoystickCame
 }
 
 const JoystickCameraStates::JoystickCameraState*
-    JoystickCameraStates::getJoystickCameraState(const std::string& joystickName) const
+JoystickCameraStates::joystickCameraState(const std::string& joystickName) const
 {
     for (const JoystickCameraState& joystickCameraState : _joystickCameraStates) {
         if (joystickCameraState.joystickName == joystickName) {
@@ -381,14 +380,14 @@ const JoystickCameraStates::JoystickCameraState*
 }
 
 JoystickCameraStates::JoystickCameraState*
-    JoystickCameraStates::findOrAddJoystickCameraState(const std::string& joystickName)
+JoystickCameraStates::findOrAddJoystickCameraState(const std::string& joystickName)
 {
-    JoystickCameraState* joystickCameraState = getJoystickCameraState(joystickName);
-    if (!joystickCameraState) {
+    JoystickCameraState* joystick = joystickCameraState(joystickName);
+    if (!joystick) {
         if (_joystickCameraStates.size() < JoystickInputStates::MaxNumJoysticks) {
             _joystickCameraStates.push_back(JoystickCameraState());
-            joystickCameraState = &_joystickCameraStates.back();
-            joystickCameraState->joystickName = joystickName;
+            joystick = &_joystickCameraStates.back();
+            joystick->joystickName = joystickName;
         }
         else {
             LWARNING(fmt::format("Cannot add more joysticks, only {} joysticks are "
@@ -396,7 +395,7 @@ JoystickCameraStates::JoystickCameraState*
             return nullptr;
         }
     }
-    return joystickCameraState;
+    return joystick;
 }
 
 

src/interaction/joystickinputstate.cpp

@@ -36,7 +36,7 @@ namespace openspace::interaction {
 float JoystickInputStates::axis(const std::string& joystickName, int axis) const {
     ghoul_precondition(axis >= 0, "axis must be 0 or positive");
 
-    if(joystickName.empty()) {
+    if (joystickName.empty()) {
         float res = std::accumulate(
             begin(),
             end(),
@@ -63,7 +63,7 @@ float JoystickInputStates::axis(const std::string& joystickName, int axis) const
     }
 
     if (!state) {
-        return 0.0f;
+        return 0.f;
     }
 
     return state->axes[axis];
@@ -72,7 +72,7 @@ float JoystickInputStates::axis(const std::string& joystickName, int axis) const
 bool JoystickInputStates::button(const std::string& joystickName, int button, JoystickAction action) const {
     ghoul_precondition(button >= 0, "button must be 0 or positive");
 
-    if(joystickName.empty()) {
+    if (joystickName.empty()) {
         bool res = std::any_of(
             begin(),
             end(),

src/navigation/navigationhandler.cpp

@@ -498,7 +498,7 @@ void NavigationHandler::loadNavigationState(const std::string& filepath) {
     }
 }
 
-void NavigationHandler::setJoystickAxisMapping(const std::string& joystickName, int axis,
+void NavigationHandler::setJoystickAxisMapping(std::string joystickName, int axis,
                                                JoystickCameraStates::AxisType mapping,
                                             JoystickCameraStates::AxisInvert shouldInvert,
                                           JoystickCameraStates::JoystickType joystickType,
@@ -506,7 +506,7 @@ void NavigationHandler::setJoystickAxisMapping(const std::string& joystickName,
                                                double sensitivity)
 {
     _orbitalNavigator.joystickStates().setAxisMapping(
-        joystickName,
+        std::move(joystickName),
         axis,
         mapping,
         shouldInvert,
@@ -516,17 +516,17 @@ void NavigationHandler::setJoystickAxisMapping(const std::string& joystickName,
     );
 }
 
-void NavigationHandler::setJoystickAxisMappingProperty(const std::string& joystickName,
+void NavigationHandler::setJoystickAxisMappingProperty(std::string joystickName,
                                                        int axis,
-                                                       const std::string& propertyUri,
+                                                       std::string propertyUri,
                                                        float min, float max,
                                             JoystickCameraStates::AxisInvert shouldInvert,
                                                        bool isRemote)
 {
     _orbitalNavigator.joystickStates().setAxisMappingProperty(
-        joystickName,
+        std::move(joystickName),
         axis,
-        propertyUri,
+        std::move(propertyUri),
         min,
         max,
         shouldInvert,

src/navigation/navigationhandler_lua.inl

@@ -164,7 +164,7 @@ int bindJoystickAxis(lua_State* L) {
     joystickType = joystickType.value_or("JoystickLike");
 
     global::navigationHandler->setJoystickAxisMapping(
-        joystickName,
+        std::move(joystickName),
         axis,
         ghoul::from_string<interaction::JoystickCameraStates::AxisType>(axisType),
         interaction::JoystickCameraStates::AxisInvert(*shouldInvert),
@@ -188,9 +188,9 @@ int bindJoystickAxisProperty(lua_State* L) {
     isRemote = isRemote.value_or(true);
 
     global::navigationHandler->setJoystickAxisMappingProperty(
-        joystickName,
+        std::move(joystickName),
         axis,
-        propertyUri,
+        std::move(propertyUri),
         *min,
         *max,
         interaction::JoystickCameraStates::AxisInvert(*shouldInvert),