-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
abstract post-processing filter factory via context #12425
Conversation
// These filters do not accept settings (nor are settings recorded in ros_writer) | ||
(void *)&settings; | ||
|
||
if( rsutils::string::nocase_equal( name, "Decimation Filter" ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this code better than before?
Performance is worse (comparing all strings)
If you make a string mistake it compile but dont work
Magic strings are used (no header that group all)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code compared strings before, too.
Yes, before they were "hard-coded" in that they were generated from the extension names. But we don't want to use the extension names. If you prefer, I can compare block names and fall back to "automatic" extension name comparison which was case-sensitive, but I don't think that's readable or easy to understand at all.
This is the factory. It centralizes the knowledge of processing blocks, and lets us add processing blocks dynamically in different modules. I like this much better than having extension names spread all over the place, many times not in any human-preferable way ("Hdr Merge" rather than "HDR Merge").
I can change to fit your preference, but the factory idea is much better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as performance, I can change to a map if you like. But we compared all strings before, too. This happens once during device creation.
Today, if you want to return "recommended processing blocks" (AKA recommended filters in the API), then you need to return the actual instantiation of the blocks.
This means that everyone needs to know how to instantiate them: the rosbag reader; the DDS device; etc.
The problem is that, if we add new filters (or remove old ones), the code needs to be updated everywhere.
The problem is that there's no abstraction of a factory that can instantiate a filter by name, that would allow a piece of code to know about the factory rather than the block implementations.
This adds such a factory, with the goal of separating the DDS/ROS/whatever code from post-processing (which may not even exist!).
pp_block_factory
pp
== post-processing, to denote these are used for post-processingblock
designation because it returnsprocessing_block_interface
processing_block_factory
that's used for more-internal non-filter stuff, so I couldn't call it what it should be calledrscore_pp_block_factory
which thecontext
knows aboutcontext::create_pp_block()
- the context will know what factories exist and how to use them; nobody else shouldcreate_pp_block()
accepts JSON settings that can be used to serialize settings; these aren't used right nowTracked on [LRS-967]