-
Notifications
You must be signed in to change notification settings - Fork 23
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
#2 solved #15
#2 solved #15
Conversation
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.
Hallo,
ich habe den Code noch nicht ausgeführt. Im Code habe ich erste Kommentare hinterlassen, bin mir aber nicht sicher ob ich den Ansatz komplett verstanden habe. Kannst du bitte einen Screenshot beifügen?
string trailColor = default("#000000ff"); // the color of the trail indicator in hex RRGGBBAA format | ||
bool waypointsShown = default(true); // decides whether or not to display waypoints |
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.
Warum "shown"? waypointsShow
oder waypoints
scheint mir passender
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.
Ich bevorzuge es boolean Variablen passend zum if Statement zu formulieren, typischerweise geht das mit einem is_ am Anfang, z.B. isWaypointShown. Da hier aber mehrere angezeigt werden fand ich die Mehrzahl passender und areWaypointsShown klingt etwas komisch. Da ich mich sehr an der Trail-Implementierung orientiert habe, ist waypoints die Sammlung/der Vector mit den Waypointlocations.
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.
Ich verstehe, find ich einen guten Grundgedanken. Dann hätte ich wohl "showWaypoints" gewählt aber ich vermute du wolltest eine gemeinsame Basis für alle waypoints Variablen.
Okay wir können den Namen unverändert lassen 👍
MobileNode.cc
Outdated
if (waypointsShown) { | ||
if (not waypoints.empty()) waypoints.clear(); | ||
CommandQueue* missionCommands = extractCommands(); | ||
for (std::deque<Command*>::iterator it = missionCommands->begin(); it != missionCommands->end(); it++) { |
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.
Ich würde vorschlagen die Anzahl der gezeichneten Waypoints zu begrenzen. Die Hälfte fänd ich okay
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.
Werde ich einbauen, dann ergibt es allerdings (geschlossenen) keinen Kreis.
MobileNode.cc
Outdated
@@ -99,6 +114,24 @@ void MobileNode::handleMessage(cMessage *msg) | |||
delete msg; | |||
msg = nullptr; | |||
} | |||
else if (msg->isName("startMission")) { |
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.
Hier wird davon ausgegangen, dass sich die Mission nicht ändert. Ich will nicht sagen dass das zwingend der Fall ist, dennoch würde ich vorschlagen das Neuzeichnen bspw bei jedem nextCommand auszuführen.
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.
Hmm, die Zeichnung wird ja in MobileNode:84 void MobileNode::refreshDisplay() const
ausgeführt, lediglich die Liste der Waypoints wird zu Missionsbeginn abgefragt. Ich konnte nicht erkennen, dass Missionen während der Ausführung geändert werden konnten.
Shown waypoint length can now be configured and will be updated upon new command loading ChargeNode now also has a message string
Anzeige der Waypoints nach commit #16 |
Hallo Michael, Ich habe soeben den letzten Branch von @LudwigBr gemerged und damit hier einen kleinen Konflikt erzeugt. Nachdem du diesen behoben hast steht dem Merge nichts mehr im Weg. |
Looks good 👍 Thanks for the addition! |
Closes #2
A line is drawn that "visits" all waypoints of the current mission.