Add several interfaces about point cloud etc#127
Conversation
| */ | ||
| enum class KeyframeFileType { | ||
| TDF, /**< TDF format */ | ||
| UNDEFINED = 1000, |
There was a problem hiding this comment.
Any reason not to define UNDEFINED first, and removing the =1000 ?
If UNDEFINED is declared first, new values could be added without changing its numerical representation.
| * @enum keyframe file type | ||
| */ | ||
| enum class KeyframeFileType { | ||
| TDF, /**< TDF format */ |
There was a problem hiding this comment.
Could you give it a less brand-releated and more descriptive name?
| /// @brief Return the text definition (string) of a KeyframeFileType object | ||
| /// @param[in] keyframeFileType the keyframe file type | ||
| /// @return the text definition (string) | ||
| static std::string toString(const KeyframeFileType& keyframeFileType) |
There was a problem hiding this comment.
nit: I guess enums can be passed by value
| /// @brief Return the text definition (string) of a KeyframeFileType object | ||
| /// @param[in] keyframeFileType the keyframe file type | ||
| /// @return the text definition (string) | ||
| static std::string toString(const KeyframeFileType& keyframeFileType) |
There was a problem hiding this comment.
Since you already use std::optional for parseKeyframeFileType() , why not using it here as well, instead of returning "Unknown value", so that caller can easily check if there was a problem, instead of checking this code to see against which string value it has to compare the result?
| /** | ||
| * @enum keyframe file type | ||
| */ | ||
| enum class KeyframeFileType { |
There was a problem hiding this comment.
KeyframeFileType does not appear in IKeyframeLoader API (or anywhere else).
Does it make sense to define it here?
Unless it needs to be referenced elsewhere, like datastructure::DescriptorType in Map.h.
We could have an implementation module component that defines this, or have a component implementing this interface by type, for example.
| // we use m_keyframesManager to access the keyframes and optimize their poses | ||
| // the modifs on keyframes' poses will not be applied to the keyframe collection of m_map | ||
| m_keyframeCollection = keyframeCollection; | ||
| if (!m_keyframeCollection) { |
There was a problem hiding this comment.
Here I don't remember exactly how it is supposed to work, because it's a bit complicated so I'm just wondering:
- does
m_embedKeyframeImages == truemean that we have images for ALL Keyframes? Or is it possible that some KF don't have images associated with them (because of Map fusion for example,...). - If so, does the fact that the KF collection we're passing here does not have any image actually mean that this map should have its flag
m_embedKeyframeImages == false? Or could it be a particular case?
My question is: is this flag related to the fact that this KF collection has images, is it actually more a decision on whether or not we should serialize images, whether it has them or not?
There was a problem hiding this comment.
I think that this flag is only used to indicate if KF collection has images, not related to serialization. m_embedKeyframeImages == true means that at least one of the keyframes in the map have image data
There was a problem hiding this comment.
- This seems to literally contradict the comment you placed just above, no?
// m_embedKeyframeImages is only used in serialization to decide if we serialize images- Also, there's this method which tends to indicate what I was referring to, that is, it's a flag activated by the user of the map to decide whether it is supposed to have images or not, not by a detection of the presence of a Keyframe image.
void Map::embedKeyframeImages()
{
m_embedKeyframeImages = true;
}- I didn't see a reference where the existence of an image would toggle this flag to
true(in Map.cpp, at least).
This being said, I'll let you the final word on this.
2 participants
No description provided.