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

Snap to grid #60

Merged
merged 28 commits into from
Apr 25, 2020
Merged

Snap to grid #60

merged 28 commits into from
Apr 25, 2020

Conversation

JShep1
Copy link

@JShep1 JShep1 commented Apr 15, 2020

In order to keep consistency of the snapping behavior to the visual grid, we'll need to make an update to the grid so that an odd cell count won't shift the grid to be misaligned with the true world coordinate frame. That issue is documented here

Ready for review

@chapulina
Copy link
Contributor

@osrf-jenkins run tests

John Shepherd and others added 21 commits April 21, 2020 20:17
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
Signed-off-by: John Shepherd <[email protected]>
@JShep1
Copy link
Author

JShep1 commented Apr 22, 2020

Manually set DCO to pass, I ran into a lot of headache trying to get the signoffs on past commits and not completely sure what's going on. I just didn't want to muddy the commit history anymore with all the rebasing.

@JShep1 JShep1 requested a review from iche033 April 22, 2020 17:16
@JShep1
Copy link
Author

JShep1 commented Apr 22, 2020

snaptogrid

Signed-off-by: John Shepherd <[email protected]>
@@ -1,6 +1,8 @@
gz_add_gui_plugin(TransformControl
SOURCES TransformControl.cc
QT_HEADERS TransformControl.hh
PUBLIC_LINK_LIBS
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be PRIVATE?

#include <ignition/rendering/Visual.hh>
#include <ignition/rendering/Geometry.hh>
#include <ignition/rendering/Grid.hh>
#include <ignition/rendering/Material.hh>
Copy link
Contributor

Choose a reason for hiding this comment

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

Material.hh not needed

/////////////////////////////////////////////////
void TransformControl::OnSnapToGrid()
{
this->dataPtr->snapToGrid = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

I notice that the tool button is not checkable . Is it possible to toggle snap to grid?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't think this functionality should be a toggle. Clicking the button will simply update the snap values to the grid cell length. Introducing a toggle snap to grid may add extra complications to the custom snap dialog - it would need to be set to read only to the user while the button is toggled on, I didn't necessarily want to introduce more logic that might in the end confuse the user (ie, a novel user might wonder why they can't update the custom snap values if they have the snap to grid button pressed or forget they have it pressed), so I thought that a simple click to set values to the visual grid would be the most intuitive approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah I see! I didn't realize clicking on the button also updated the custom snap values. Not sure what the best way is to give feedback

a different issue: while testing the snapping tool, I noticed that if the model is selected with the translation tool active, the snapping behavior does not work the first time but works the second time, e.g.

  1. click model
  2. select translation tool button -> arrows now displayed over the model
  3. click snap to grid button
  4. Hold ctrl and drag arrow -> no snapping
  5. Release ctrl key and mouse button
  6. Repeat 4. -> snapping works.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't realize clicking on the button also updated the custom snap values

@chapulina and I decided that it would likely be best to keep the Custom Snap Value dialog consistent with the current snapping values so that when the snap the grid button is clicked, the internal snap values are updated to the grid cell length and then reflected in the dialog.

I noticed that if the model is selected with the translation tool active, the snapping behavior does not work the first time but works the second time, e.g.

That's actually been a recurring issue that isn't necessarily tied to this PR, this behavior occurs in other cases and has been around since keybindings were added, essentially when the "scene" window is not prioritized and the CTRL (or any other button, in fact) is pressed, Scene3D does not register the button press, which is where all the logic for those functionalities is held. This is an artifact of QML's event hierarchy and likely needs a separate issue, Louise and I did some researching into this and couldn't quite find a good solution.

Copy link
Contributor

@iche033 iche033 Apr 24, 2020

Choose a reason for hiding this comment

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

Got it, that makes sense now. The render window only gets focus only when the mouse is pressed:
https://github.com/ignitionrobotics/ign-gazebo/blob/master/src/gui/plugins/scene3d/Scene3D.cc#L2244

So I think we don't get any key events before dragging the model with the mouse. Maybe we have to force focus to be active when the mouse hovers over the render window. Anyways that's a separate issue to tackle.

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