Skip to content
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

Feat static tree #37

Open
wants to merge 2 commits into
base: noetic-devel
Choose a base branch
from
Open

Conversation

ipa-jsk
Copy link
Collaborator

@ipa-jsk ipa-jsk commented Nov 11, 2024

Use static tf tree (tf2) instead of publishing with fix rate.
Closes #14

transforms = []
for element in elements:
if element is not None and (level & 1 == 1 or level & 4 == 4):
print(f"updating element {element.name}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
print(f"updating element {element.name}")

@@ -308,7 +308,7 @@ def parse_args(self, argv):
parser.add_argument("-l", "--load", action="append",
dest="file",
help="Load a file at startup. [rospack filepath/file]")
parser.add_argument("-r", "--rate", type=int)
parser.add_argument("-r", "--rate", type=int, help="Rate for broadcasting. Does not involve tf frames.")
Copy link
Contributor

@ipa-danb ipa-danb Dec 3, 2024

Choose a reason for hiding this comment

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

Suggested change
parser.add_argument("-r", "--rate", type=int, help="Rate for broadcasting. Does not involve tf frames.")
parser.add_argument("-r", "--rate", type=int, help="Rate for broadcasting. Does not involve tf frames. This argument is deprecated and will be removed in future versions.")

Copy link
Contributor

Choose a reason for hiding this comment

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

Would also be nice to add a warning in line 319

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is it deprecated? One could still use it.

@ipa-danb
Copy link
Contributor

ipa-danb commented Dec 4, 2024

Does this break the "delete" feature as it is implemented right now? Delete Frames will still be latched and wont vanish if you delete them?

@ipa-jsk
Copy link
Collaborator Author

ipa-jsk commented Dec 4, 2024

Does this break the "delete" feature as it is implemented right now? Delete Frames will still be latched and wont vanish if you delete them?

This is true indeed.. well, with the current non-static publisher, the frames also are not deleted, but just not published anymore, such that a lookup causes an error after some time. This would not be possible with static trees.. see also this issue:

ros/geometry2#370

The delete feature still has the problem that you cannot create a frame with the same name again. So, its actually almost useless. Could also be an argument to get rid of the delete feature?

We could still implement both variants (static and non-static) as you suggested earlier, if there is any value in the deletion feature.

@ipa-danb
Copy link
Contributor

ipa-danb commented Dec 4, 2024

Does this break the "delete" feature as it is implemented right now? Delete Frames will still be latched and wont vanish if you delete them?

This is true indeed.. well, with the current non-static publisher, the frames also are not deleted, but just not published anymore, such that a lookup causes an error after some time. This would not be possible with static trees.. see also this issue:

ros/geometry2#370

The delete feature still has the problem that you cannot create a frame with the same name again. So, its actually almost useless. Could also be an argument to get rid of the delete feature?

We could still implement both variants (static and non-static) as you suggested earlier, if there is any value in the deletion feature.

I mean the delete referes to deleting it from the frame editor, not from the TF tree. The only problem with the deleting is just that you cannot reassign it if it already exists in the TF tree. With the current implementation, i see it as a bug that you cannot reassign a frame. We can fix it by clearing the frame buffer ( as implemented in #39 ). For static Transformer we would need a new concept for saveguarding reassignment, e.g., by tracking which frame has ever been published by the frame editor (and allow assignment then).

@ipa-jsk
Copy link
Collaborator Author

ipa-jsk commented Dec 9, 2024

The only problem with the deleting is just that you cannot reassign it if it already exists in the TF tree.

Are you sure? I thought you can just publish a new transform. I'll test.

@ipa-danb
Copy link
Contributor

ipa-danb commented Dec 9, 2024

The only problem with the deleting is just that you cannot reassign it if it already exists in the TF tree.

Are you sure? I thought you can just publish a new transform. I'll test.

The issue lies with the implementation in frame editor. it checks for frames in the buffer and only allows for ones that are not set

    def btn_add_clicked(self, checked):

       existing_frames = set(self.editor.all_frame_ids())

        name, ok = QtWidgets.QInputDialog.getText(self.widget, "Add New Frame", "Name:", QtWidgets.QLineEdit.Normal, "my_frame");

        while ok and name in existing_frames:
            name, ok = QtWidgets.QInputDialog.getText(self.widget, "Add New Frame", "Name (must be unique):", QtWidgets.QLineEdit.Normal, "my_frame")
        if not ok:
            return

Which makes sense because it prevents from publishing transform that jump around

@ipa-jsk
Copy link
Collaborator Author

ipa-jsk commented Dec 10, 2024

-> idea: Check which frames are static and allow only reassigning them

@ipa-jsk
Copy link
Collaborator Author

ipa-jsk commented Jan 8, 2025

-> idea: Check which frames are static and allow only reassigning them

@ipa-danb check new commit. It was more difficult than expected. This was the smartest way I could find.

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.

Use tf2 static frames
2 participants