-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
clicking.mp4EDITED: clicking2.mp4 |
There was a problem hiding this 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." |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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*>(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be const?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can _mouseEvent
be const?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: QT -> Qt
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Task 5 of #377 (comment)
This PR unblocks tasks 6(Arrow mesh) and 7(Lane selector) given that they need raycast support.