Skip to content

Conversation

@pazeshun
Copy link
Contributor

@pazeshun pazeshun commented Jul 11, 2025

With this PR, you can switch on Reset When Timed Out function, which makes image display show "No Image" again when new image has not been received for Timeout seconds.
This PR makes it easier to notice that the image publisher is in trouble.

Before this PR

Screencast.2025-07-11.19.46.35.compressed.mp4

After this PR

Screencast.2025-07-11.19.51.45.compressed.mp4

Note that the default state of Reset When Timed Out is OFF, which leads to the same behavior before this PR.

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Interesting feature, but the implementation seems to be overly complicated.
You are running a periodic QTimer and update the timeout_time_ in its associated slot. Instead, you could simply update a last_received_ timestamp within incomingMessage() and directly call reset() in the slot function.
Are you fearing data races?

{
if (reset_to_property_->getBool())
{
reset_to_timer_->start(33);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this fixed timeout value? How is it motivated?

@rhaschke rhaschke changed the base branch from noetic-devel to main August 7, 2025 22:56
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Some more comments.

@rhaschke rhaschke merged commit 116c6a9 into ros-visualization:main Aug 26, 2025
7 of 8 checks passed
pazeshun added a commit to pazeshun/rviz that referenced this pull request Oct 2, 2025
@pazeshun pazeshun deleted the no-img-again branch October 2, 2025 09:38
@pazeshun
Copy link
Contributor Author

pazeshun commented Oct 2, 2025

@rhaschke Thank you very much for your support, and I am deeply sorry for my super late response.
I confirmed your changes perfectly work in our usage.
One thing I should mention is that the current default behavior (Timeout is 1.0) is different from the default behavior before this PR.
If you consider this is problem, please merge #1856.
Anyway, thank you very much again.

@rhaschke
Copy link
Contributor

rhaschke commented Oct 2, 2025

I changed the default behavior deliberatively to report issues after 1s.

@pazeshun
Copy link
Contributor Author

pazeshun commented Oct 2, 2025

I see. Thank you very much!

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.

2 participants