Feature/update track mode #149
Conversation
|
Automatic code review is enabled for this repository. Reviewing this PR now. |
![devloai[bot]](https://avatars.githubusercontent.com/in/886113?s=60&v=4){kind=small}
There was a problem hiding this comment.
I've reviewed the track mode updates and found several areas for improvement while maintaining the core functionality. The changes include good enhancements to the tracking system, particularly the configurable parameters for smoothing and detection. The main suggestions focus on API consistency, documentation accuracy, and code maintainability. Please address the method name typo and documentation issues as they affect the public API. The code organization suggestions will help with long-term maintenance.
| @@ -159,6 +159,21 @@ class INSPIRE_API FaceTrackModule { | |||
| */ | |||
| bool IsDetectModeLandmark() const; | |||
|
|
|||
| /** | |||
| * @brief Fix detect threshold | |||
There was a problem hiding this comment.
The documentation comments for these methods incorrectly state "Fix detect threshold". Please update them to accurately describe their purpose. For example:
For SetTrackModeSmoothRatio:
/**
* @brief Sets the smoothing ratio for landmark tracking
* @param value Smoothing ratio between 0 and 1, smaller values mean stronger smoothing
*/For SetTrackModeNumSmoothCacheFrame:
/**
* @brief Sets the number of frames to use for landmark smoothing
* @param value Number of previous frames to consider for smoothing
*/| /** | ||
| * @brief Fix detect interval | ||
| * */ | ||
| void SetTrackModelDetectInterval(int value); |
There was a problem hiding this comment.
The method name has a typo - "Model" should be "Mode":
| void SetTrackModelDetectInterval(int value); | |
| void SetTrackModeDetectInterval(int value); |
| @@ -186,8 +186,22 @@ bool FaceTrackModule::TrackFace(inspirecv::InspireImageProcess &image, FaceObjec | |||
| // INSPIRE_LOGD("Extensive Affine Cost %f", extensive_cost_time.GetCostTimeUpdate()); | |||
| } | |||
| } | |||
| // Replace the landmark with the high-quality landmark | |||
There was a problem hiding this comment.
Consider extracting the landmark mapping into a separate method to improve readability and maintainability. The same mapping is done twice (once for setting and once for getting smoothed landmarks). Something like:
void UpdateHighQualityLandmarks(FaceObjectInternal& face, const std::vector<inspirecv::Point2f>& landmarks) {
landmarks[FaceLandmarkAdapt::LEFT_EYE_CENTER] = face.high_result.lmk[0];
landmarks[FaceLandmarkAdapt::RIGHT_EYE_CENTER] = face.high_result.lmk[1];
landmarks[FaceLandmarkAdapt::NOSE_CORNER] = face.high_result.lmk[2];
landmarks[FaceLandmarkAdapt::MOUTH_LEFT_CORNER] = face.high_result.lmk[3];
landmarks[FaceLandmarkAdapt::MOUTH_RIGHT_CORNER] = face.high_result.lmk[4];
}| @@ -28,13 +28,14 @@ class INSPIRE_API FaceObjectInternal { | |||
| face_action_ = std::make_shared<FaceActionPredictor>(10); | |||
| } | |||
|
|
|||
| void SetLandmark(const std::vector<inspirecv::Point2f> &lmk, bool update_rect = true, bool update_matrix = true) { | |||
| void SetLandmark(const std::vector<inspirecv::Point2f> &lmk, bool update_rect = true, bool update_matrix = true, float h = 0.06f, int n = 5) { | |||
There was a problem hiding this comment.
The default values for h and n parameters in SetLandmark method are different from the class member defaults in FaceTrackModule (0.06 vs 0.025 and 5 vs 15). Consider:
- Using consistent defaults across the codebase
- Adding validation for these parameters (e.g. h should be between 0 and 1, n should be positive)
- Adding documentation for what these parameters mean and their valid ranges
| @@ -205,7 +219,8 @@ void FaceTrackModule::UpdateStream(inspirecv::InspireImageProcess &image) { | |||
| detection_index_ += 1; | |||
| if (m_mode_ == DETECT_MODE_ALWAYS_DETECT || m_mode_ == DETECT_MODE_TRACK_BY_DETECT) | |||
| trackingFace.clear(); | |||
| if (detection_index_ % detection_interval_ == 0 || m_mode_ == DETECT_MODE_ALWAYS_DETECT || m_mode_ == DETECT_MODE_TRACK_BY_DETECT) { | |||
| if (trackingFace.empty() || detection_index_ % detection_interval_ == 0 || m_mode_ == DETECT_MODE_ALWAYS_DETECT || | |||
There was a problem hiding this comment.
The condition for detection has become quite long and complex. Consider extracting it to a method with a descriptive name for better readability:
bool shouldPerformDetection() const {
return trackingFace.empty() ||
detection_index_ % detection_interval_ == 0 ||
m_mode_ == DETECT_MODE_ALWAYS_DETECT ||
m_mode_ == DETECT_MODE_TRACK_BY_DETECT;
}
No description provided.