Make viewer capable to load rules files and tree visualization of phases #224

BMarchi · 2019-08-21T17:23:51Z

Part of #207

hidmic

First pass. We should get base PRs going.

hidmic · 2019-08-23T17:21:26Z

delphyne-gui/visualizer/layer_selection_widget.cc

+}
+
+void MaliputFileSelectionWidget::onTrafficLightrulesButtonPressed() {
+  QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),


@BMarchi nit: consider

Suggested change

QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),

QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook YAML"),

as you did above.

hidmic · 2019-08-23T17:21:50Z

delphyne-gui/visualizer/layer_selection_widget.cc

+void MaliputFileSelectionWidget::onTrafficLightrulesButtonPressed() {
+  QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),
+                                                  QString::fromStdString(this->fileDialogOpenPath),
+                                                  tr("All files (*.*)"));


@BMarchi shouldn't it be YAML files?

I'm not entirely sure if all files should be YAML to be honest

They are, at least for the time being and the foreseeable future.

hidmic · 2019-08-23T17:23:10Z

delphyne-gui/visualizer/layer_selection_widget.cc

+}
+
+void MaliputFileSelectionWidget::onPhaseRingButtonPressed() {
+  QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),


@BMarchi Phase Ring Rulebook YAML?

hidmic · 2019-08-23T17:26:32Z

delphyne-gui/visualizer/layer_selection_widget.cc

+void MaliputFileSelectionWidget::onPhaseRingButtonPressed() {
+  QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),
+                                                  QString::fromStdString(this->fileDialogOpenPath),
+                                                  tr("All files (*.*)"));


@BMarchi shouldn't it be YAML files?

hidmic · 2019-08-23T18:37:23Z

delphyne-gui/visualizer/maliput_viewer_model.cc

-          _maliputFilePath.substr(_maliputFilePath.find_last_of("/") + 1), _maliputFilePath);
+          _maliputFilePath.substr(_maliputFilePath.find_last_of("/") + 1), _maliputFilePath, _roadRulebookFilePath,
+          _trafficLightBookFilePath, _phaseRingFilePath);
+      fileStream.close();


@BMarchi there's no need to explicitly close the stream, its destructor will.

hidmic · 2019-08-23T18:42:32Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+    std::vector<QString>>&& phases_per_ring) {
+  DELPHYNE_DEMAND(this->phase_tree != nullptr);
+  this->phase_tree->clear();
+  ignerr << phases_per_ring.size() << std::endl;


@BMarchi leftover?

hidmic · 2019-08-23T18:42:44Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+  ignerr << phases_per_ring.size() << std::endl;
+  for (auto& phase_ring_it : phases_per_ring) {
+    QTreeWidgetItem* phase_ring_item = new QTreeWidgetItem(this->phase_tree);
+    ignerr << phase_ring_it.first << std::endl;


@BMarchi leftover?

hidmic · 2019-08-23T18:44:26Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+    for (size_t i = 0; i < phases.size(); ++i) {
+      QTreeWidgetItem* phase_item = new QTreeWidgetItem();
+      phase_item->setText(0, std::move(phases[i]));
+      phase_ring_item->addChild(phase_item);


@BMarchi is there a way to let the phase_ring_item manage the memory allocation?

What do you mean? I'm almost sure that once the item is added, it should be cleaned by the tree

Right, but if something bad happens at the previous line memory will be leaked. Thus my question. It's fine if the answer is no.

Something bad like what? I would suppose that setText doesn't throw exceptions

I would suppose that setText doesn't throw exceptions

Hmm, I'd prefer would suppose to be just know :)

Quoting Qt:

Qt itself will not throw exceptions. Instead, error codes are used.

This doesn't return a code error, which I'll say nothing will happen.

hidmic · 2019-08-23T18:45:23Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+}
+
+void RulesVisualizerWidget::ConstructPhaseRingTree(std::unordered_map<std::string,
+    std::vector<QString>>&& phases_per_ring) {


@BMarchi fyi: you may also pass by value. As long as the provided argument is an rvalue, the move constructor will be used to set it up.

I also want to pass an l-value just in case, that's why I'm using a universal reference

Hmm, doesn't that mean that:

std::unordered_map<std::string, std::vector<QString>> phases_per_ring widget.ConstructPhaseRingTree(phases_per_ring);

is valid? If so, phases_per_ring is emptied in ConstructPhaseRingTree and there's no indication of it whatsoever (besides documentation of course). Wouldn't it be better to have ConstructPhaseRingTree(std::move(phases_per_ring))? At least it is explicit.

It's valid and that way we enforce the user to explicitly consider that whatever he's using will be moved. If he wants to preserve whatever he has, he will need to copy the container (which will be the same if we weren't moving the data). I don't want to leave the possibility of copying the parameter.

That's fine I guess. But please do update ConstructPhaseRingTree documentation about these side effects.

hidmic · 2019-08-23T18:51:15Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+    // |_PhaseRing2
+    // |__Phase1
+    QTreeWidgetItem* parent = selected_phase->parent();
+    if (parent)


@BMarchi hmm, what does it mean to select a phase ring? IIUC phases belong to rings, but that's about it. They bear ~~other~~ no meaning w.r.t. rules.

Select only a phase ring doesn't mean anything. But I need it for the query. I think I'm not following you.

Had a typo in my comment, sorry about that. My point is, if there's no meaning to a phase ring being selected, why allowing it in the first place?

I'm not allowing it. That's the point of the if condition. The signal is fired whenever I click on the phase ring or the phase alone.

That I understand. But if we don't allow it, there's no need to have code dealing with it. Why not using QTreeWidgetItem::SetFlags, specifically Qt::ItemIsSelectable in Qt::ItemFlags? Sorry for not being explicit enough.

Also, std::pair is somewhat terse. Can we have a struct? Or simply looking up the Phase* in the PhaseRingBook could work too.

But if we don't allow it, there's no need to have code dealing with it. Why not using QTreeWidgetItem::SetFlags, specifically Qt::ItemIsSelectable in Qt::ItemFlags? Sorry for not being explicit enough.

I got confused a little bit. I'm not connecting the "Selected" signal from the tree. I get the currentItem which provides me the item I clicked no matter what. I tried to turning off Qt::ItemIsSelectable bit I still get the phase ring item. I suspect that the signal in the tree doesn't get called, but the clicked index does get saved internally anyway.

Hmm, would using selectedItems() method instead solve the problem?

hidmic · 2019-08-26T16:43:40Z

delphyne-gui/visualizer/layer_selection_widget.cc

+void MaliputFileSelectionWidget::onRoadRulebookButtonPressed() {
+  QString fileName = QFileDialog::getOpenFileName(this, tr("Open Road Rulebook YAML"),
+                                                  QString::fromStdString(this->fileDialogOpenPath),
+                                                  tr("YAML files (*.YAML)"));


@BMarchi nit²: *.YAML -> *.yaml here and below.

hidmic · 2019-08-26T16:44:44Z

delphyne-gui/visualizer/maliput_viewer_model.hh

@@ -322,7 +333,30 @@ ContainerType MaliputViewerModel::GetAllLaneIds() const {
 }

 template <typename StringType>
-StringType MaliputViewerModel::GetRulesOfLane(const std::string& _laneId) const {
+std::unordered_map<std::string, std::vector<StringType>> MaliputViewerModel::GetPhaseRings() const {


@BMarchi something I had not noticed before, why not using StringType for a map key type?

I didn't want to force the implementation of a hash for the string type. QString is not supported for std::unordered_map

Hmm I see. That's fine.

hidmic · 2019-08-26T16:46:03Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+    std::vector<QString>>&& phases_per_ring) {
+  DELPHYNE_DEMAND(this->phase_tree != nullptr);
+  this->phase_tree->clear();
+  for (auto& phase_ring_it : phases_per_ring) {


@BMarchi nit: phase_ring_n_phases? It's not an iterator.

hidmic · 2019-08-26T16:50:52Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+}
+
+void RulesVisualizerWidget::ConstructPhaseRingTree(std::unordered_map<std::string,
+    std::vector<QString>>&& phases_per_ring) {


Hmm, doesn't that mean that:

std::unordered_map<std::string, std::vector<QString>> phases_per_ring widget.ConstructPhaseRingTree(phases_per_ring);

is valid? If so, phases_per_ring is emptied in ConstructPhaseRingTree and there's no indication of it whatsoever (besides documentation of course). Wouldn't it be better to have ConstructPhaseRingTree(std::move(phases_per_ring))? At least it is explicit.

hidmic

Hmm, thinking about this from another perspective, why not simply having a single slot that is connected to lane and phase ID update signals and then retrieves that information instead of getting it via signal arguments? Clicking the scene would then update the lane selection which in turn would update the rules.

I bring this up because I see room for simplification if we cascade some updates instead of them being direct.

hidmic · 2019-08-28T14:54:00Z

delphyne-gui/visualizer/maliput_viewer_model.hh

+  for (const auto& phase_ring_id : phase_ring_ids) {
+    drake::optional<maliput::api::rules::PhaseRing> phase_ring = phase_ring_book->GetPhaseRing(phase_ring_id);
+    const std::unordered_map<maliput::api::rules::Phase::Id, maliput::api::rules::Phase>& phases =
+        phase_ring.value().phases();


@BMarchi should we DELPHYNE_DEMAND that phase_ring.has_value()?

I guess. I assumed that it should have at least one phase.

hidmic · 2019-08-28T14:55:23Z

delphyne-gui/visualizer/maliput_viewer_widget.cc

@@ -53,7 +55,9 @@ void MaliputViewerWidget::OnTextLabelChanged(const std::string& key, bool newVal
 }

 /////////////////////////////////////////////////
-void MaliputViewerWidget::OnNewMultilaneFile(const std::string& filePath) {
+void MaliputViewerWidget::OnNewMultilaneFile(const std::string& filePath, const std::string& roadRulebookFilePath,


@BMarchi nit: now that we've added so much functionality, maybe this callback should be OnNewRoadNetwork instead?

hidmic · 2019-08-28T15:01:13Z

delphyne-gui/visualizer/rules_visualizer_widget.hh

@@ -34,19 +40,25 @@ class RulesVisualizerWidget : public QWidget {
  void ClearLaneList();
  /// \brief Clear the text in TextBrowser.
  void ClearText();
+  /// \brief Construct the tree for the given phase_ring. Beware that the parameter will be moved. Caller will have


@BMarchi consider using a \warning or a \post command for Beware... and so on.

hidmic · 2019-08-28T15:05:11Z

delphyne-gui/visualizer/maliput_viewer_widget.cc

  this->rulesVisualizerWiget->ClearText();
  this->rulesVisualizerWiget->ClearLaneList();
+  this->rulesVisualizerWiget->ConstructPhaseRingTree(this->model->GetPhaseRings<QString>());


@BMarchi typo: rulesVisualizerWidget

hidmic · 2019-08-28T15:06:36Z

delphyne-gui/visualizer/maliput_viewer_widget.cc

-      OnRulesForLaneRequested(QString(lane_id.c_str()));
+      PhaseRingPhaseIds phaseRingIdAndPhaseIdSelected = this->rulesVisualizerWiget->GetSelectedPhaseRingAndPhaseId();
+      OnRulesForLaneRequested(phaseRingIdAndPhaseIdSelected.phase_ring_id, phaseRingIdAndPhaseIdSelected.phase_id,
+                              QString(lane_id.c_str()));


@BMarchi why do we call the slot here instead of emitting the signal?

Even if the slot is emiting the signal only, that could change. I prefer to make use of the same method instead.

BMarchi · 2019-08-28T17:43:42Z

Hmm, thinking about this from another perspective, why not simply having a single slot that is connected to lane and phase ID update signals and then retrieves that information instead of getting it via signal arguments? Clicking the scene would then update the lane selection which in turn would update the rules.

I didn't understand anything. A single slot where?

hidmic · 2019-08-28T20:29:59Z

I see that we call the rules request slot manually here and through a connected signal here. In both cases, we retrieve phase and lane IDs to call that slot.

Now, imagine you had a single slot that doesn't get phase and lane IDs as arguments but looks for them itself. If you connect it to signals, you can trigger it whenever you want to update the rules being listed. That'd simplify the code e.g. you would be able to collapse GetSelectedPhaseRingAndPhaseId() implementation into that slot, and it scales better as you further refine the search with other IDs, matching filters and such (i.e. no need to replicate the request, just send a signal after UI has been updated).

Sorry for not being explicit enough. What do you think?

hidmic

LGTM

hidmic · 2019-08-30T20:02:34Z

delphyne-gui/visualizer/layer_selection_widget.cc

+  if (lastIndexOf != -1) {
+    return filePath.mid(lastIndexOf + 1);
+  }
+  return QString();


@BMarchi could it be thatfilePath has no slashes? Also, don't we have filesystem utilities elsewhere we can reuse?

I'll double check and use them if they exist

hidmic · 2019-08-30T20:07:20Z

delphyne-gui/visualizer/rules_visualizer_widget.cc

+}
+
+QString RulesVisualizerWidget::GetSelectedLaneId() const {
+  QListWidgetItem* currentItem = this->lanes_list->currentItem();


@BMarchi should it be selectedItem?

hidmic

LGTM

hidmic · 2019-09-02T14:39:44Z

delphyne-gui/visualizer/layer_selection_widget.cc

+void MaliputFileSelectionWidget::ClearLineEdits(bool keepOldTextsAsPlaceholders) {
+  if (keepOldTextsAsPlaceholders) {
+    std::string roadRulebookFileName = ignition::common::basename(this->roadRulebookLineEdit->text().toStdString());
+    this->roadRulebookLineEdit->setPlaceholderText(QString::fromStdString(roadRulebookFileName));


@BMarchi nit: no need to change this, but FYI it looks like Qt has this QFileInfo class that could preclude the need to for these QString -> std::string -> QString transformations.

hidmic · 2019-09-02T14:43:58Z

delphyne-gui/visualizer/maliput_viewer_model.hh

+    rules += "[Right of way rules by phase ring id and phase id]\n" +
+             GetPhaseRightOfWayRules<StringType>(maliput::api::rules::PhaseRing::Id(_phaseRingId),
+                                                 maliput::api::rules::Phase::Id(_phaseId)) +
+             "\n";


@BMarchi nit: we could use std::ostringstream here as we do elsewhere. It's more efficient and portable.

That implies to erase all template methods since we are not going to return any string and then convert it to a QString. If you want that, I'll change it then.

I just saw that you did something quite similar below for another method. My comment still stands, but it's just a nit.

Yeah, the stringstream is used at the moment because the Query class use that. But this method in particular just gathers all the rules that were parsed with that class

hidmic

LGTM

hidmic

LGTM

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

hidmic suggested changes Aug 23, 2019

View reviewed changes

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

BMarchi force-pushed the bmarchi/add_tree_visualization_for_phases branch from 32dc66f to db56c20 Compare August 26, 2019 13:53

hidmic reviewed Aug 26, 2019

View reviewed changes

hidmic reviewed Aug 28, 2019

View reviewed changes

BMarchi force-pushed the bmarchi/add_tree_visualization_for_phases branch from c6c082c to 696f8f4 Compare August 30, 2019 12:26

BMarchi requested a review from hidmic August 30, 2019 12:59

hidmic previously approved these changes Aug 30, 2019

View reviewed changes

BMarchi dismissed hidmic’s stale review via c75c7a2 September 2, 2019 12:53

hidmic previously approved these changes Sep 2, 2019

View reviewed changes

BMarchi dismissed hidmic’s stale review via d8caad7 September 2, 2019 15:02

hidmic previously approved these changes Sep 3, 2019

View reviewed changes

BMarchi added 6 commits September 3, 2019 13:57

Make viewer capable to load rules files and tree visualization of phases

05aa091

Keep file names of rules in line edits as placeholders

a62cb1a

Address pr last comments

a9dbe13

Make use of QFileInfo for file paths

09e00d2

Run clang format

396ce2e

Updated with upstream changes

0a32fa4

BMarchi dismissed hidmic’s stale review via 0a32fa4 September 3, 2019 17:07

BMarchi force-pushed the bmarchi/add_tree_visualization_for_phases branch from 88092a8 to 0a32fa4 Compare September 3, 2019 17:07

hidmic approved these changes Sep 3, 2019

View reviewed changes

BMarchi merged commit 2bfd62b into master Sep 4, 2019

BMarchi deleted the bmarchi/add_tree_visualization_for_phases branch September 4, 2019 16:05

	QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook"),
	QString fileName = QFileDialog::getOpenFileName(this, tr("Open Traffic Lights Rulebook YAML"),

Make viewer capable to load rules files and tree visualization of phases #224

Make viewer capable to load rules files and tree visualization of phases #224

Conversation

BMarchi commented Aug 21, 2019 • edited by francocipollone Loading

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

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 Aug 26, 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

hidmic Aug 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMarchi Aug 26, 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

BMarchi Aug 26, 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

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

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

BMarchi commented Aug 28, 2019

hidmic commented Aug 28, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BMarchi Sep 2, 2019 • edited Loading

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

Choose a reason for hiding this comment

BMarchi commented Aug 21, 2019 •

edited by francocipollone

Loading

hidmic Aug 26, 2019 •

edited

Loading

hidmic Aug 23, 2019 •

edited

Loading

BMarchi Aug 26, 2019 •

edited

Loading

BMarchi Aug 26, 2019 •

edited

Loading

BMarchi Sep 2, 2019 •

edited

Loading