Added array parameters #37
Conversation
now: double array parameter send after finish edit any element and no started edit another element
There was a problem hiding this comment.
Hi! Thank you for your contribution, array parameters are one of the big missing features in this tool. Thanks for being patient with us while waiting for a review.
I looked at your video, the functionality looks good, although the column with for double[]_param seems to not match the others, perhaps that can be fixed.
Not sending partial array updates also sounds useful, i think that would be expected behavior. Note to self and @authaldo : we should really look into some visual indication of node-state and display-state to make that even more obvious to the user, i think we brainstormed methods for this before but didnt come to a nice conclusion...
I did leave some notes on the implementation, i think keeping UI state well contained is important to keep the code modular and readable, and i would welcome suggestions by @authaldo .
The table generation seems a bit repetitive, but if it isn't easy to extract common functionality in a function, keep it that way.
The code style doesn't quite match the existing style, but since we don't have any .clang-format file yet (should be the clion defaults), i dont think thats important. Please do clean up redundant parentheses in a bunch of places though.
There was a problem hiding this comment.
Please remove the vscode config files, maybe add .vscode to the gitignore, since it contains user-specific file paths and settings.
There was a problem hiding this comment.
I think those are some forgotten notes from testing, please remove this file as well
| struct BoolArrayParam : ArrayParam<bool> {}; | ||
| struct IntArrayParam : ArrayParam<long int> {}; | ||
| struct DoubleArrayParam : ArrayParam<double> {}; | ||
| struct StringArrayParam : ArrayParam<std::string> {}; |
There was a problem hiding this comment.
this is missing the byte-array parameter type. i'm ok with intentionally omitting this in the assumption that it was designed for BLOBs, but we should note that here in that case
| bool isChanging; | ||
| bool isChanged; |
There was a problem hiding this comment.
I will have to look into the code again to refresh my memory on how ROSParameterVariant is used, but combining the parameter value with additional state here seems unintuitive, maybe @authaldo has opinions on that
| @@ -155,10 +155,13 @@ std::set<ImGuiID> visualizeParameters(ServiceWrapper &serviceWrapper, | |||
|
|
|||
| ImGui::SameLine(); | |||
| ImGui::PushItemWidth(static_cast<float>(textfieldWidth)); | |||
| static ImGuiTableFlags flags = ImGuiTableFlags_ScrollX | ImGuiTableFlags_ScrollY | ImGuiTableFlags_BordersInnerV | ImGuiTableFlags_Reorderable | ImGuiTableFlags_Hideable | ImGuiTableFlags_NoHostExtendX; | |||
There was a problem hiding this comment.
i think this is a constant? if so, please make it constexpr and put it next to the other constants at the start of this file. if this is mutable, we should find a better place than a static variable deep inside this function
| @@ -199,7 +202,150 @@ std::set<ImGuiID> visualizeParameters(ServiceWrapper &serviceWrapper, | |||
| serviceWrapper.pushRequest( | |||
| std::make_shared<ParameterModificationRequest>(ROSParameter(fullPath, value))); | |||
| } | |||
| } | |||
| } else if (std::holds_alternative<BoolArrayParam>(value)) { | |||
| if (ImGui::BeginTable(identifier.c_str(), (std::get<BoolArrayParam>(value)).arrayValue.size(), flags, outer_size)) | |||
There was a problem hiding this comment.
| if (ImGui::BeginTable(identifier.c_str(), (std::get<BoolArrayParam>(value)).arrayValue.size(), flags, outer_size)) | |
| if (ImGui::BeginTable(identifier.c_str(), std::get<BoolArrayParam>(value).arrayValue.size(), flags, outer_size)) |
| ImGui::TableNextColumn(); | ||
| ImGui::PushID(cell); | ||
| ImGui::SetNextItemWidth(-FLT_MIN); | ||
| ImGui::DragScalar(identifier.c_str(), ImGuiDataType_S64, &((std::get<IntArrayParam>(value)).arrayValue.at(cell)), -1); |
There was a problem hiding this comment.
above we use DragInt instead of DragScalar for integers, i don't know what the difference is but we should probably keep that consistent
| BoolArrayParam arrParam; | ||
| arrParam.isChanging = false; | ||
| arrParam.isChanged = false; | ||
| arrParam.arrayValue = valueMsg.bool_array_value; | ||
| ROSParameterVariant v = arrParam; |
There was a problem hiding this comment.
if we keep the state and value combined in the parameter type, we should at least give *ArrayParam a constructor which initializes isChanging and isChanged to false, to avoid having to repeat that multiple times here. But i'm still not convinced of keeping (UI) state here...
| @@ -11,6 +11,7 @@ | |||
| #include "service_wrapper.hpp" | |||
| #include <chrono> | |||
| #include <regex> | |||
| #include <typeinfo> | |||
There was a problem hiding this comment.
i think this is unused
| #include <typeinfo> |
Array parameters are built into the standard get/set algorithm. The parameters are displayed in table form. The indexes of the array elements are displayed. There is support for horizontal table scrolling.The parameter is sent if any table element was edited and after that another table element was not selected. Therefore it is possible to change multiple elements in an array in one send. Table cell width automatically resizes based on text length.
rig_reconfigure demo video.zip