Skip to content

Conversation

@guilyx
Copy link
Contributor

@guilyx guilyx commented Dec 6, 2021

When remapping a topic foo to bar, it appears that Subscriber::getTopic returns the name of the topic as initialized when instantiating the subscriber foo.
In the base class, get topic returns the remapped topic bar, which is in my opinion the correct behavior.

If I were to retrieve the topic I'm subscribing to, I expect to retrieve the remapped name (aka the true name) rather than something that is not used.

gbiggs
gbiggs previously approved these changes Feb 19, 2022
@gbiggs gbiggs dismissed their stale review February 19, 2022 06:03

Mistaken click

@gbiggs
Copy link
Member

gbiggs commented Feb 19, 2022

@guilyx Could you please add a test that verifies the new behaviour?

@guilyx
Copy link
Contributor Author

guilyx commented Feb 25, 2022

@gbiggs Sure, will take a few days

@audrow audrow changed the base branch from master to rolling June 28, 2022 14:19
@CursedRock17
Copy link
Contributor

Hey all, ran into this issue today and this PR would be helpful to prevent it from happening to others. I wrote up two tests on a separate branch to verify it. Would be more beneficial to open a new PR, or add onto this one?

@guilyx
Copy link
Contributor Author

guilyx commented Jan 10, 2026

Thanks for commenting, I forgot this existed (and was never fixed apparently) I could simply merge your branch into mine ?
Let me know!

@CursedRock17
Copy link
Contributor

Sure thing, did you want to rebase your branch first because of the merge conflict? Then I'll shoot the PR over to your branch.

@guilyx
Copy link
Contributor Author

guilyx commented Jan 10, 2026

Yea I need to see, I haven't looked at this in 4Y. As soon as I grab a laptop today I will likely rebase + ping you

@guilyx guilyx force-pushed the get-topic-name-w-remap branch from 36115a4 to 6c615c1 Compare January 12, 2026 05:39
@guilyx
Copy link
Contributor Author

guilyx commented Jan 12, 2026

@CursedRock17 all yours. rebased on my fork and added you as a contrib just in case you may not be able to push there. let me know!

@guilyx
Copy link
Contributor Author

guilyx commented Jan 13, 2026

@gbiggs PR should be ready!

CursedRock17 and others added 2 commits January 13, 2026 16:34
Co-authored-by: Alejandro Hernández Cordero <[email protected]>
Signed-off-by: Lucas Wendland <[email protected]>
@ahcorde
Copy link
Contributor

ahcorde commented Jan 14, 2026

Pulls: #68
Gist: https://gist.githubusercontent.com/ahcorde/b6e99f11203285c87a3541f4dd03866e/raw/8c1990b377f2ff6947cb0cf25a72ddfb71c3f62d/ros2.repos
BUILD args: --packages-above-and-dependencies message_filters
TEST args: --packages-above message_filters
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/17945

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

@ahcorde ahcorde merged commit 5cc2fb6 into ros2:rolling Jan 14, 2026
2 checks passed
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.

4 participants