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

Tweak focal shape size marker depending on focal distance #1021

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Tweak focal shape size marker depending on focal distance #1021

merged 1 commit into from
Oct 17, 2016

Conversation

VictorLamoine
Copy link
Contributor

@VictorLamoine VictorLamoine commented Jun 7, 2016

Fixes #1020

This PR adds two options:

  • Setting the focal shape size.
  • A zoom invariant mode so that the focal shape size does not change when zooming in/out.

The only problem with this PR is the initial shape size; I found no way of knowing the focal distance before the user interacts (zooms in or out) so the shape size at the start is a little bit arbitrary.

Fixed size:

old

Zoom invariant:

new

@trainman419
Copy link
Contributor

When zooming or panning an empty world, the focal shape is often my only visual cue about what the current zoom level is. Having the focal shape scale dynamically counteracts this.

A simpler approach that maintains the functionality that I use on a regular basis would be to make the size of the focal shape settable by the user.

@de-vri-es
Copy link
Contributor

Or both, and a checkbox to correct for zoom level (the size should then be the size at 100% zoom)

@wjwwood
Copy link
Member

wjwwood commented Jun 10, 2016

@VictorLamoine makes sense to me, but I also see @trainman419's point of view. How hard would it be to add an option to the display menu? Are you interested in doing that?

Also, I added gif's of your before and after to your OP.

@VictorLamoine
Copy link
Contributor Author

VictorLamoine commented Jun 10, 2016

I don't know how hard it can be because I don't know the code, but I will at least give it a try!

I will try to add an option in the display menu:

  • Fixed sized focal shape
  • Zoom-invariant focal shape size

For both of them we will be able to choose a size for the shape.

@VictorLamoine
Copy link
Contributor Author

VictorLamoine commented Jun 10, 2016

How hard would it be to add an option to the display menu?

@wjwwood You probably meant the Views panel, right?

I added a user option to be able to choose the shape size, I still need to add a menu allowing to choose between fixed size/zoom invariant.

@wjwwood
Copy link
Member

wjwwood commented Jun 10, 2016

I was originally meaning the display panel, which has the displayed types and the grid and stuff like that, but I think the View panel makes more sense now that you mention it.

@VictorLamoine
Copy link
Contributor Author

VictorLamoine commented Jun 10, 2016

Ready for review!

view_panel

@wjwwood
Copy link
Member

wjwwood commented Jun 10, 2016

Thanks! I'll have a look at it this weekend.

@wjwwood
Copy link
Member

wjwwood commented Jun 21, 2016

Just tried it out, but I've noticed that the changes in the settings do not take effect until you change the zoom:

rviz_focal_point

@VictorLamoine
Copy link
Contributor Author

There are two things that can be improved:

  • Behavior at start-up: the distance_property_ is not set until the user zooms in or out so there is no way of setting the right focal shape size on start-up
  • When the user changes the focal shape size it is not updated until the user zooms in/out.

For the first item we need to find a way to correctly initialize the distance property value.

For the second item we could probably connect the Focal Shape Size valueChanged() signal to a updateFocalShapeSize slot. (same applies to the check box).

I will look at that and get back to you!

@wjwwood
Copy link
Member

wjwwood commented Jun 24, 2016

I will look at that and get back to you!

Sounds like a plan. Let me know if I can help with questions or testing.

@VictorLamoine
Copy link
Contributor Author

VictorLamoine commented Aug 29, 2016

All problems are solved; updating the focal shape in mouse events solves the two problems.
Tell me if the code is ok with you, when it is I will expand the modifications to the other view controllers.

@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

Sorry for the delay, I'll just tried it out real quick and it looks great now. The only thing is that on my machine the pip is now teal or blueish. Did you change that too?

@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

It doesn't look like it... If I don't use the navigation tutorial to launch rviz it is yellow... That must be a different bug.

@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

Looks like this happens on kinetic-devel too, so it won't block this pr.

Thanks again for the contribution, it's a great feature!

@wjwwood wjwwood merged commit c7d6179 into ros-visualization:kinetic-devel Oct 17, 2016
@wjwwood
Copy link
Member

wjwwood commented Oct 17, 2016

#1056

130s pushed a commit to 130s/rviz that referenced this pull request Aug 21, 2024
Signed-off-by: Alejandro Hernández Cordero <ahcorde@gmail.com>
(cherry picked from commit 092e3efef2f907549976ffd101e5ad8100cbea3f)
130s pushed a commit to 130s/rviz that referenced this pull request Aug 21, 2024
…/pr-1021

Remove warning in depth_cloud_mld.cpp (backport ros-visualization#1021)
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