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

MaliputViewerPlugin: Provides ray-cast support #388

Merged
merged 4 commits into from
Apr 29, 2021

Conversation

francocipollone
Copy link
Collaborator

@francocipollone francocipollone commented Apr 28, 2021

Task 5 of #377 (comment)

  • Hook to the mouse event handler
  • ray-cast: From a screen coordinate get the visual that intercepts that ray.

This PR unblocks tasks 6(Arrow mesh) and 7(Lane selector) given that they need raycast support.

@francocipollone francocipollone marked this pull request as draft April 28, 2021 17:57
@francocipollone
Copy link
Collaborator Author

francocipollone commented Apr 28, 2021

clicking.mp4

EDITED:

clicking2.mp4

@francocipollone francocipollone marked this pull request as ready for review April 29, 2021 13:25
@francocipollone francocipollone changed the title WIP: Provides ray-cast support MaliputViewerPlugin: Provides ray-cast support Apr 29, 2021
Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

PTAL

Just a handful of comments about style. Nothing against the feature implementation.

}
}
if (!scene3D) {
ignerr << "Scene3D plugin titled '" << kMainScene3dPlugin << "' wasn't found. MouseEvents won't be filtered."
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't it be a definitive error? I recommend returning and flagging and error or raising an exception.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -340,6 +347,58 @@ void MaliputViewerPlugin::ConfigurateScene() {
directionalLight->SetDiffuseColor(lightRed, lightGreen, lightBlue);
directionalLight->SetSpecularColor(lightRed, lightGreen, lightBlue);
rootVisual->AddChild(directionalLight);

// Install event filter to get mouse event from the main scene.
QList<Plugin*> plugins = parent()->findChildren<Plugin*>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we convert this block in a function that filters plugins by title?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


bool MaliputViewerPlugin::eventFilter(QObject* _obj, QEvent* _event) {
if (_event->type() == QEvent::Type::MouseButtonPress) {
QMouseEvent* mouseEvent = static_cast<QMouseEvent*>(_event);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can it be const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

return QObject::eventFilter(_obj, _event);
}

void MaliputViewerPlugin::MouseClickHandler(QMouseEvent* _mouseEvent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can _mouseEvent be const?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.


void MaliputViewerPlugin::MouseClickHandler(QMouseEvent* _mouseEvent) {
const auto rayQueryResult = ScreenToScene(_mouseEvent->x(), _mouseEvent->y());
if (rayQueryResult.distance > -1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume that distance being less than zero is an error (i.e. no collision)? Can we test for non-negative values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! My bad. Done

const maliput::api::Lane* lane = model->GetLaneFromWorldPosition(rayQueryResult.point);
if (lane) {
const std::string& lane_id = lane->id().string();
ignmsg << "Clicked lane ID: " << lane_id << std::endl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't you do: ignmsg << "Clicked lane ID: " << lane->id().string() << std::endl;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure. Done.

}
}

ignition::rendering::RayQueryResult MaliputViewerPlugin::ScreenToScene(int screenX, int screenY) const {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you missed _ in variable names.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

/// the scene is not ready yet.
void timerEvent(QTimerEvent* _event) override;

/// \brief Intercepts an @p _event delivered to other object @p _obj.
/// \details We are particularly interested in QMouseEvents happening within the main scene.
/// This method is called automatically by the QT Event System iff the event filter was installed
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: QT -> Qt

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

/// the scene is not ready yet.
void timerEvent(QTimerEvent* _event) override;

/// \brief Intercepts an @p _event delivered to other object @p _obj.
/// \details We are particularly interested in QMouseEvents happening within the main scene.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would rephrase this to say:

Filters QMouseEvents from a Scene3D plugin whose title is set via <scene_plugin_title>.
To make this method be called by Qt Event System, install the event filter in target object
( \see QObject::installEventFilter() method).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

@@ -82,6 +90,9 @@ class MaliputViewerPlugin : public ignition::gui::Plugin {
/// @brief The scene name.
static constexpr char const* kSceneName = "scene";

/// @brief The Scene3D instance holding the main scene.
static constexpr char const* kMainScene3dPlugin = "Main3DScene";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's make this a plugin argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

And change occurrences of kMainScene3dPlugin to refer the xml argument.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should also update the plugin documentation to state what it is expected as configuration.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agree. Done.

Copy link
Collaborator

@agalbachicar agalbachicar left a comment

Choose a reason for hiding this comment

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

LGTM

@francocipollone francocipollone merged commit 3c885a0 into master Apr 29, 2021
@francocipollone francocipollone deleted the francocipollone/raycast_support branch April 29, 2021 21:14
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