Skip to content

Conversation

@rom-thom
Copy link

@rom-thom rom-thom commented Jan 7, 2026

Refactoring the structure of the image filtering to give it more functionality trough classes. Updated Readme and comenting

jorgenfj and others added 30 commits May 26, 2024 15:33
ci: add ci workflows and config files
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.9.2 → v0.11.4](astral-sh/ruff-pre-commit@v0.9.2...v0.11.4)
- [github.com/pre-commit/mirrors-clang-format: v19.1.7 → v20.1.0](pre-commit/mirrors-clang-format@v19.1.7...v20.1.0)
- [github.com/codespell-project/codespell: v2.3.0 → v2.4.1](codespell-project/codespell@v2.3.0...v2.4.1)

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
updates:
- [github.com/astral-sh/ruff-pre-commit: v0.11.4 → v0.12.2](astral-sh/ruff-pre-commit@v0.11.4...v0.12.2)
- [github.com/pre-commit/mirrors-clang-format: v20.1.0 → v20.1.7](pre-commit/mirrors-clang-format@v20.1.0...v20.1.7)
@codecov
Copy link

codecov bot commented Jan 7, 2026

Codecov Report

❌ Patch coverage is 0% with 370 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (1451a74) to head (e8d5e0f).

Files with missing lines Patch % Lines
image-filtering/src/image_filtering_ros.cpp 0.00% 134 Missing ⚠️
image-filtering/src/utilities.cpp 0.00% 130 Missing ⚠️
image-filtering/src/image_processing.cpp 0.00% 76 Missing ⚠️
...ltering/include/image_filters/image_processing.hpp 0.00% 30 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main     #17    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files          3       5     +2     
  Lines        234     464   +230     
  Branches      18      35    +17     
======================================
- Misses       234     464   +230     
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ring/include/image_filters/image_filtering_ros.hpp 0.00% <ø> (ø)
...ltering/include/image_filters/image_processing.hpp 0.00% <0.00%> (ø)
image-filtering/src/image_processing.cpp 0.00% <0.00%> (ø)
image-filtering/src/utilities.cpp 0.00% <0.00%> (ø)
image-filtering/src/image_filtering_ros.cpp 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rom-thom rom-thom requested review from jorgenfj and kluge7 and removed request for jorgenfj January 8, 2026 14:43
@jorgenfj
Copy link
Contributor

jorgenfj commented Jan 8, 2026

template<typename T>
T declare_and_get(
    const std::string& name,
    const T& default_value)
{
    if (!this->has_parameter(name)) {
        this->declare_parameter<T>(name, default_value);
    }
    return this->get_parameter(name).as<T>();
}

Can something like this be used to avoid having to declare all parameters.
Then the declare_parameters function only has to declare the topics names and filter type and this declare_and_get function can be used in set_filter_params

@jorgenfj
Copy link
Contributor

jorgenfj commented Jan 8, 2026

Also remove the new filters that are not present on the main branch. This PR should just be a refactor, not add filters

@jorgenfj
Copy link
Contributor

jorgenfj commented Jan 8, 2026

Add namespace vortex::image_filtering to header files

Copy link
Contributor

@jorgenfj jorgenfj left a comment

Choose a reason for hiding this comment

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

You should look into splitting up ros and non-ros code in subfolders /lib and /ros and then export /lib as a separate library so that other users can directly import filters into their code. An example of this setup in terms of folder structure and Cmake is here pose_filtering.

Also remove the "Todo" and "examples" from the actual code. It is enough that it is documented in the README how you implement new filters. That should be fine as vortexers have gigachad IQ

Comment on lines 63 to 65
std::cout << "\033[33m No string connected to that filter type: '" << s
<< "'. This might be misspelling or you need to add the filter "
"type to kFilterMap in image_processing.hpp\033[0m";
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using spdlog instead. spdlog::warn will return red warning messages.

Comment on lines 32 to 76
static constexpr std::pair<std::string_view, FilterType> kFilterMap[] = {
{"no_filter", FilterType::NoFilter},
{"flip", FilterType::Flip},
{"unsharpening", FilterType::Unsharpening},
{"erosion", FilterType::Erosion},
{"dilation", FilterType::Dilation},
{"white_balancing", FilterType::WhiteBalancing},
{"ebus", FilterType::Ebus},
{"otsu", FilterType::Otsu},
{"overlap", FilterType::Overlap},
{"median_binary", FilterType::MedianBinary},
{"binary", FilterType::Binary},

// TODO(Vortex): Also add your filter here
{"example", FilterType::Example},
{"unknown", FilterType::Unknown}};

inline std::string to_lower(std::string s) {
for (char& ch : s) {
ch = static_cast<char>(std::tolower(static_cast<unsigned char>(ch)));
}
return s;
}

inline FilterType parse_filter_type(std::string s) {
s = to_lower(std::move(s));

for (auto [name, type] : kFilterMap) {
if (s == name)
return type;
}
std::cout << "\033[33m No string connected to that filter type: '" << s
<< "'. This might be misspelling or you need to add the filter "
"type to kFilterMap in image_processing.hpp\033[0m";
return FilterType::Unknown;
}

inline std::string_view filtertype_to_string(FilterType t) {
for (auto [name, type] : kFilterMap) {
if (t == type)
return name;
}
std::cout << "\033[33m No string connected to your filter type. To fix "
"this add the string and filter type to kFilterMap\033[0m";
return "unknown";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can move kFilterMap, to_lower, parse_filter_type, filtertype_to_string be moved to a util header file

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe to a different header than the one where you keep the image util functions

Comment on lines 69 to 77
inline std::string_view filtertype_to_string(FilterType t) {
for (auto [name, type] : kFilterMap) {
if (t == type)
return name;
}
std::cout << "\033[33m No string connected to your filter type. To fix "
"this add the string and filter type to kFilterMap\033[0m";
return "unknown";
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be needed. For the filtertype setup where this is used, just return early in default case/clause, print the string ros parameter instead of calling this function after the switch case.

Comment on lines +125 to +127
struct FlipParams {
int flip_code;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you create an enum class for the different flip codes

Comment on lines 64 to +65
void ImageFilteringNode::set_filter_params() {
FilterParams params;
std::string filter =
std::string filter_type_string =
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe create small helper functions to construct the filter types and place them in a ros utils header file

Comment on lines 246 to 247
void ImageFilteringNode::check_and_subscribe_to_image_topic() {
std::string image_topic = this->get_parameter("sub_topic").as_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a similar function for the publisher

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants