feat: adding JsonKindDispatcher + Surface conversion#5195
feat: adding JsonKindDispatcher + Surface conversion#5195ssdetlab wants to merge 12 commits intoacts-project:mainfrom
Conversation
|
Introduced an additional overload for boundless surfaces to unify the interface |
| } | ||
|
|
||
| template <typename surface_t> | ||
| std::string getSurfaceKind() { |
| /// @return a shared object created from json input | ||
| static std::shared_ptr<Surface> fromJson(const nlohmann::json& jSurface); | ||
|
|
||
| static Config m_cfg; |
There was a problem hiding this comment.
This being static means that it can't be extended at this time, right?
Maybe this should also be private.
There was a problem hiding this comment.
Made it private. Added a setter for the time being. When the migration to the dispatcher pattern is finished, we can just make a regular constructor for this
| } | ||
|
|
||
| if constexpr (std::is_void_v<return_t>) { | ||
| decoder->second(encoded, std::forward<args_t>(args)...); |
There was a problem hiding this comment.
This would only forward if the function was
template <typename... Ts>
return_t operator()(const nlohmann::json& encoded, Ts&&... args) const {so the template function needs to accept Ts&&, only then are they universal references and can be forwarded. I think we probably want to do that.
There was a problem hiding this comment.
Made this a forwarding reference
| /// | ||
| /// @return string representation of the surface bounds kind | ||
| template <typename bounds_t> | ||
| std::string getSurfaceBoundsKind() { |
There was a problem hiding this comment.
I think this function and the other templated functions are only used in the .cpp file. Can you move them there instead of having them in the header here? That would also allow removing the added headers and moving them into the .cpp only.
There was a problem hiding this comment.
Moved the templates to the cpp. Cleaned up the headers
|
|
||
| #include <memory> | ||
|
|
||
| #include <nlohmann/json_fwd.hpp> |
There was a problem hiding this comment.
The .cpp file should probably include the full header I suppose?
There was a problem hiding this comment.
Removed this -- clangd adds these automatically sometimes, missed this one
|
2 participants
Adding JsonKindDispatcher that unifies the interface for the json conversion.
Refactoring the Surface json conversion to TypeDispatcher + JsonKindDispatcher flow.
SurfaceJsonConverter is now a class that stores the json encoders/decoders and its Config members.
The class is static, since a lot of external conversion depends on the conversion functions being statically accessible.
Could be refactored later (e.g., refactor all the json converters to classes and pass the surface converter as a Config member).
The kind of the Surface + Bounds type is performed by two independent functions, in case there will be changes to the way the type is handled.
PerigeeSurface is handled independently outside of the dispatcher structure, since it does not follow the usual constructor argument convention.
The said constructor could be added to unify the interface.
The PR also adds GeometryIdentifier conversion with the dedicated converter and fixes the material conversion bug.