Add arrow to selected lanes #223

BMarchi · 2019-08-14T20:36:33Z

Ready for review

hidmic · 2019-08-16T20:31:01Z

delphyne-gui/visualizer/arrow_mesh.cc

+      moveDown = true;
+      currentTick = 0;
+    }
+  }


@BMarchi haha you did it. Consider turning moveDown into a an integer that can be either -1 or 1 and multiples step before summation. It'll simplify the code.

@BMarchi nit: can we call moveDown something else e.g. currentDirection ?

hidmic · 2019-08-16T20:31:35Z

delphyne-gui/visualizer/arrow_mesh.hh

+
+public:
+
+  /// \brief Constructor. Creates a cone and adds it as a child to the RootVisual of the scene, which will move


@BMarchi consider removing Constructor., it is somewhat redundant.

hidmic · 2019-08-16T20:34:29Z

delphyne-gui/visualizer/arrow_mesh.hh

+  /// \brief Moves the arrow to a given world position and resets the downwards movement.
+  /// \param[in] _distanceFromCamera How far the camera is from the clicked point.
+  /// \param[in] _worldPosition World position of the cursor ray cast click.
+  void MoveArrowAt(double _distanceFromCamera, const ignition::math::Vector3d& _worldPosition);


@BMarchi I'm not sure MoveArrowAt expresses the intent you describe in the documentation. Consider HoverOver or something along those lines.

hidmic · 2019-08-16T20:35:00Z

delphyne-gui/visualizer/arrow_mesh.hh

+  void MoveArrowAt(double _distanceFromCamera, const ignition::math::Vector3d& _worldPosition);
+  /// \brief Toggles the visibility of the arrow.
+  /// \param[in] _visible Boolean that determines if the arrow should be visible or not.
+  void SetArrowVisibility(bool _visible);


@BMarchi consider SetVisibility, Arrow is likely redundant.

hidmic · 2019-08-16T20:42:22Z

delphyne-gui/visualizer/arrow_mesh.cc

+
+  const double scaleIncrement = 1.0 + scaleFactor * _distanceFromCamera;
+  const double newMinBBZAxis = scaleIncrement * minBoundingBox.Z();
+  this->arrow->SetWorldScale(scaleIncrement, scaleIncrement, scaleIncrement);


@BMarchi I see what you're doing here, but does it scale appropriately if you zoom in/out? Should the scaling code be separate and called upon zoom?

In practice it looks fine to me. I wanted to avoid user interaction with the arrow's scale directly

Hmm, I'll take a look locally.

hidmic · 2019-08-20T13:35:17Z

delphyne-gui/visualizer/arrow_mesh.cc

+      moveDown = true;
+      currentTick = 0;
+    }
+  }


@BMarchi nit: can we call moveDown something else e.g. currentDirection ?

hidmic · 2019-08-20T13:36:29Z

delphyne-gui/visualizer/render_maliput_widget.hh

+  /// \brief Move the arrow based on the distance travelled by the camera's ray distance.
+  /// \param[in] _distance Distance travelled from the camera's world position to the clicked object.
+  /// \param[in] _worldPosition Position where the camera's ray hit.
+  void MoveArrowAt(double _distance, const ignition::math::Vector3d& _worldPosition);


@BMarchi PutArrowAt ?

hidmic · 2019-08-20T13:37:15Z

delphyne-gui/visualizer/arrow_mesh.hh

+  /// \param[in] _distanceFromCamera How far the camera is from the clicked point.
+  /// \param[in] _worldPosition World position of the cursor ray cast click.
+  void SelectAt(double _distanceFromCamera, const ignition::math::Vector3d& _worldPosition);
+  /// \brief Toggles the visibility of the arrow.


@BMarchi nit: add one extra blank line here.

hidmic · 2019-08-20T13:38:50Z

delphyne-gui/visualizer/arrow_mesh.cc

+
+  const double scaleIncrement = 1.0 + scaleFactor * _distanceFromCamera;
+  const double newMinBBZAxis = scaleIncrement * minBoundingBox.Z();
+  this->arrow->SetWorldScale(scaleIncrement, scaleIncrement, scaleIncrement);


Hmm, I'll take a look locally.

hidmic · 2019-08-20T16:32:17Z

Feedback from @liangfok and @andrewbest-tri: lower the arrow so that it lays closer to the lane surface and casted shadows become a better indicator.

liangfok · 2019-08-20T16:48:49Z

Can you post a screenshot?

BMarchi · 2019-08-22T13:25:30Z

This is how it looks

liangfok · 2019-08-22T14:01:53Z

Looks good. Thanks for the screenshot. Perhaps changing the color of the yellow edge of the lane that's selected, it'll be even more clear. When there's multiple overlapping lanes with no height difference, like at an intersection, I wander if we can click multiple times, each time changing which of the N overlapping lanes is selected.

BMarchi · 2019-08-22T15:06:37Z

For the moment, we can't do anything to a single lane, since the road is only one mesh, so changing the material will imply that the whole road will change. #218 explains the problems we have. This is meant to be a quick and cheap solution (better than nothing) until we can find out how to make a proper outline since it's not easy to do

hidmic

LGTM pending conflict resolution

hidmic · 2019-08-26T13:52:34Z

delphyne-gui/visualizer/arrow_mesh.cc

+  ignition::math::Vector3d worldPosition = this->arrow->WorldPosition();
+  this->arrow->SetWorldPosition(worldPosition.X(), worldPosition.Y(), worldPosition.Z() + currentDirection * step);
+  ++currentTick;
+  if (currentTick == totalTicks) {


@BMarchi nit: last two lines could be collapsed into one.

hidmic · 2019-08-26T16:38:28Z

delphyne-gui/visualizer/arrow_mesh.cc

+  this->cylinder->SetVisible(false);
+  this->cylinder->SetWorldPosition(0., 0., 0.);
+  // This size is enough for visualization.
+  this->cylinder->SetWorldScale(0.5, 0.5, 0.2);


@BMarchi as I told you offline, this is likely to run into some issues when dealing with very small lanes. Unless we somehow bound its size by the lane's area, which is going to be tough. Maybe once we get an outline we don't need this one anymore?

The arrow is useful but I think that with the outliner, the cylinder isn't necessary

hidmic

LGTM!

hidmic

LGTM pending green CI

hidmic

LGTM as soon as CI catches up.

BMarchi force-pushed the bmarchi/add_arrow_to_selected_lanes branch 3 times, most recently from 895035b to eaaca3e Compare August 16, 2019 17:23

hidmic suggested changes Aug 16, 2019

View reviewed changes

BMarchi force-pushed the bmarchi/add_arrow_to_selected_lanes branch from eaaca3e to 4e6372c Compare August 20, 2019 12:40

BMarchi mentioned this pull request Aug 20, 2019

Add visualization for clicked lane #218

Closed

hidmic reviewed Aug 20, 2019

View reviewed changes

BMarchi force-pushed the bmarchi/add_arrow_to_selected_lanes branch from 4e6372c to ab8d180 Compare August 20, 2019 15:09

BMarchi requested a review from hidmic August 20, 2019 15:19

hidmic mentioned this pull request Aug 20, 2019

Maliput road networks inspection #207

Open

14 tasks

BMarchi force-pushed the bmarchi/add_widget_for_rules_visualization branch from 311ba5f to bc5542d Compare August 23, 2019 11:27

hidmic previously approved these changes Aug 23, 2019

View reviewed changes

Create arrow class and move it to selected lane

1bd709c

BMarchi force-pushed the bmarchi/add_arrow_to_selected_lanes branch from 2c268b0 to 1bd709c Compare August 26, 2019 13:33

BMarchi dismissed hidmic’s stale review via 1bd709c August 26, 2019 13:34

BMarchi changed the base branch from bmarchi/add_widget_for_rules_visualization to master August 26, 2019 13:34

BMarchi requested a review from hidmic August 26, 2019 13:35

hidmic reviewed Aug 26, 2019

View reviewed changes

hidmic previously approved these changes Aug 26, 2019

View reviewed changes

Remove cylinder

ccbfdd1

BMarchi dismissed hidmic’s stale review via ccbfdd1 August 27, 2019 14:58

hidmic previously approved these changes Aug 28, 2019

View reviewed changes

Run clang format

98083d8

BMarchi dismissed hidmic’s stale review via 98083d8 August 28, 2019 17:49

hidmic approved these changes Aug 28, 2019

View reviewed changes

BMarchi merged commit 8f3f999 into master Aug 29, 2019

BMarchi deleted the bmarchi/add_arrow_to_selected_lanes branch September 18, 2019 13:17

stonier mentioned this pull request Dec 6, 2022

Maliput viewer #35

Open

16 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add arrow to selected lanes #223

Add arrow to selected lanes #223

BMarchi commented Aug 14, 2019 •

edited

Loading

hidmic Aug 16, 2019

hidmic Aug 20, 2019

hidmic Aug 16, 2019

hidmic Aug 16, 2019

hidmic Aug 16, 2019

hidmic Aug 16, 2019

BMarchi Aug 20, 2019

hidmic Aug 20, 2019

hidmic Aug 20, 2019

hidmic Aug 20, 2019

hidmic Aug 20, 2019

hidmic Aug 20, 2019

hidmic commented Aug 20, 2019

liangfok commented Aug 20, 2019

BMarchi commented Aug 22, 2019

liangfok commented Aug 22, 2019

BMarchi commented Aug 22, 2019

hidmic left a comment

hidmic Aug 26, 2019

hidmic Aug 26, 2019

BMarchi Aug 26, 2019

hidmic left a comment

hidmic left a comment •

edited

Loading

hidmic left a comment


		public:

		/// \brief Constructor. Creates a cone and adds it as a child to the RootVisual of the scene, which will move

Add arrow to selected lanes #223

Add arrow to selected lanes #223

Conversation

BMarchi commented Aug 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic commented Aug 20, 2019

liangfok commented Aug 20, 2019

BMarchi commented Aug 22, 2019

liangfok commented Aug 22, 2019

BMarchi commented Aug 22, 2019

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

hidmic left a comment • edited Loading

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

BMarchi commented Aug 14, 2019 •

edited

Loading

hidmic left a comment •

edited

Loading