Skip to content

Feature/non kalman tf fix#500

Open
CemTulgar23 wants to merge 10 commits intomainfrom
feature/non-kalman-tf-fix
Open

Feature/non kalman tf fix#500
CemTulgar23 wants to merge 10 commits intomainfrom
feature/non-kalman-tf-fix

Conversation

@CemTulgar23
Copy link
Contributor

@CemTulgar23 CemTulgar23 commented Dec 25, 2025

When creating new points from pre-existing objects like Bin, Octagon, and Gate, instead of sending these points to object_map_tf_server via the set_transform service to be processed by the Kalman filter, they are now published to the object_transform_non_kalman_create topic and directly published to the TF tree without going through the Kalman filter.

…rulan yeni frame'leri yayınlamak için olulturuldu. Yeni frame'ler oluşturulurken kalman'a sokulmuyor.
Copilot AI review requested due to automatic review settings December 25, 2025 16:05
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the transform handling mechanism for pre-existing objects (Bin, Octagon, Gate, etc.) by introducing a new non-Kalman filtered path. Instead of using the set_object_transform service which applies Kalman filtering, transforms are now published via the object_transform_non_kalman_create topic and added directly to the TF tree without filtering.

Key Changes:

  • Added new C++ callback and broadcast method to handle non-Kalman filtered transforms in object_map_tf_server
  • Refactored Python trajectory publishers to publish transforms via the new topic instead of calling the service
  • Modernized Python initialization code to use super() instead of explicit parent class calls

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
auv_smach/src/auv_smach/common.py Refactored to use super() for initialization; added unused publisher
auv_smach/src/auv_smach/bin.py Added outcomes parameter to service state
torpedo_frame_publisher.py Added non-Kalman publisher; contains critical syntax error
slalom_trajectory_publisher.py Added non-Kalman publisher; commented out service calls
pipeline_trajectory_publisher.py Added non-Kalman publisher; commented out service calls
octagon_trajectory_publisher.py Added non-Kalman publisher; commented out service calls
gps_target_frame_publisher.py Added non-Kalman publisher; contains critical syntax errors
gate_trajectory_publisher.py Added non-Kalman publisher; attribute name typo
bin_trajectory_publisher.py Added publishers with unused one; added Turkish comments
object_position_filter.cpp Added explanatory Turkish comments for Kalman filter parameters
object_map_tf_server_ros.cpp Implemented non-Kalman transform handling; large commented code block
object_map_tf_server_ros.hpp Added non-Kalman filter data structures and method declarations
slalom.yaml Removed YAML document separator

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +50 to +53
self.object_transform_pub = rospy.Publisher(
"map/object_transform_updates", TransformStamped, queue_size=10
)

Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

This publisher for "map/object_transform_updates" is created but never used. Line 80 shows it's commented out. If this publisher is not needed, it should be removed to avoid confusion.

Suggested change
self.object_transform_pub = rospy.Publisher(
"map/object_transform_updates", TransformStamped, queue_size=10
)

Copilot uses AI. Check for mistakes.

# -------------------- Helpers --------------------

void send_transform(self, transform):
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The syntax for defining a method in Python is incorrect. The keyword "void" is not valid in Python. The correct syntax should use "def" instead of "void".

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback
void locating in the python code was deleted and def started to use instead of void.

# 5) Build and send GPS target frame via service
ts = self._build_transform(self.child_frame, self.parent_frame, target_pos, q)
try:
send_transform(self, ts)
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The method is called with "self" as the first argument, but this is incorrect. In Python, when calling an instance method from within the class, "self" should not be passed as an argument - it is automatically passed. The call should be "self.send_transform(ts)" instead.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #501, to work on those changes. Once the pull request is ready, I'll request review from you.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #502, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #503, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #504, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #505, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link

Copilot AI commented Dec 25, 2025

@CemTulgar23 I've opened a new pull request, #506, to work on those changes. Once the pull request is ready, I'll request review from you.

@CemTulgar23
Copy link
Contributor Author

@copilot open a new pull request to apply changes based on the comments in this thread

Copy link

Copilot AI commented Dec 27, 2025

@CemTulgar23 I've opened a new pull request, #507, to work on those changes. Once the pull request is ready, I'll request review from you.

CemTulgar23 and others added 2 commits December 27, 2025 14:48
Syntax error solved.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Some comment lines that were created to test the code was deleted.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link

Copilot AI commented Dec 27, 2025

@CemTulgar23 I've opened a new pull request, #508, to work on those changes. Once the pull request is ready, I'll request review from you.

Copy link
Collaborator

@eminmeydanoglu eminmeydanoglu left a comment

Choose a reason for hiding this comment

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

Hey, good first PR!

The overall design for the solution looks good. I left some comments.

pose.orientation = transform.transform.rotation
return pose

# göndereceğimiz transform mesajını oluşturur. Objenin odom frame'e göre pozisyon ve oryantasyonunu ayarlar
Copy link
Collaborator

Choose a reason for hiding this comment

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

All code and comments in english, por favor :D

Comment on lines 25 to 41
@@ -30,6 +34,11 @@ class ObjectMapTFServerROS {
private:
void broadcast_transforms();

void broadcast_non_kalman_transforms();

void object_transform_non_kalman_create_callback(
const geometry_msgs::TransformStamped::ConstPtr &msg);

Copy link
Collaborator

Choose a reason for hiding this comment

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

The name "non-kalman" is confusing, this is just static transform publishing, dont need the term "kalman" in it?

Maybe something like static_broadcasts.

you can ask ai for recommendations.

Comment on lines 54 to 56
self.object_non_kalman_transform_pub = rospy.Publisher(
"object_transform_non_kalman_create", TransformStamped, queue_size=10
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, confusing topic name.

double distance_threshold_;
node_handler_private.param<double>("distance_threshold", distance_threshold_,
4.0);
4.0); // 4.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

redundant comment

Comment on lines +114 to +115
ROS_DEBUG_STREAM("Stored static transform from " << static_frame_ << " to "
<< target_frame);
Copy link
Collaborator

Choose a reason for hiding this comment

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

nice debug.

@senceryazici
Copy link
Member

@CemTulgar23 good work. When describing a feature please also include why this feature is being added.

For example, "this feature is added since there are certain objects that are detected with enough precision that we decided kalman filter makes it even worse" etc.

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.

5 participants