adding back files to be reviewed #1

tessavdheiden · 2019-07-01T12:31:49Z

The code in the folder "visualizer" should not be reviewed. The "implementation/include" is where I created an interface "planner.h" and the implementation of the algorithm "implementation.h" and "implementation.cpp".

pullrequest

✅ A review job has been created and sent to the PullRequest network.

Check the status or cancel PullRequest code review here - or - cancel by adding [!pr] to the title of the pull request.

pullrequest

Nitpicks mostly here.

I added CMake changes that may be beneficial to you.

Definitely want to add -Wall -Wextra to the compile options. Warnings are your friend.

Was this helpful? Yes | No

pullrequest · 2019-07-01T19:02:44Z

README.md

+
+Moving downwards decreases the path time and upwards increases it. We can model this by changing the time with some X %. However, travelling upwards and downwards, may not be equally as coslty as on flat areas, so upwards should increase the time with some extra percentage.
+
+Below we see the path planned with a small car (top picture) and a heavy car (bottum picture). The more "weight" the car has, the bigger the difference between going up vs going down for the path time. So a heavy car tries to avoid the hills more than a smaller car. 


bottom picture

pullrequest · 2019-07-01T19:02:44Z

implementation/include/implementation.h

+
+    void print_coordinate(std::pair<int, int> a);
+
+    template<typename Location, typename Graph>


It would make more sense to flip the order of these arguments. template<typename Graph, typename Location>. I prefer following the order of the function arguments.

pullrequest · 2019-07-01T19:02:44Z

README.md

+
+<img src="results/dijkstra.png" width="256" height="256" title="Dijkstra">
+
+A* is a modification of Dijkstra’s Algorithm that is optimized for a single destination. Dijkstra’s Algorithm can find paths to all locations; A* finds paths to one location, or the closest of several locations. It prioritizes paths that seem to be leading closer to a goal. The picture below shows the path planned by A*.


The asterisk in A* may need to be escaped with a backslash to prevent markdown from taking it as a 'begin bold text' signal

pullrequest · 2019-07-01T19:02:44Z

main.cpp

+enum {
+    IMAGE_DIM = 2048, // Width and height of the elevation and overrides image
+
+    ROVER_X = 159,


It might make sense to create a struct Size { int width; int height; } and work with these.

You might consider also making a Point struct that you can then use with donut and scatter.

These structs are actually very, very common and it makes sense to introduce types for them.

const kBachelorSize = Size(1303, 85);

pullrequest · 2019-07-01T19:02:44Z

README.md

+<img src="results/dijkstra_heavy.png" width="256" height="256" title="Dijkstra">
+
+### Improvements
+- The planner could be implemented with a Builder pattern. An algorithm (Dijktra or A*) receives a type of graph (grid or weighted grid) and a model (small or big). 


pullrequest · 2019-07-01T19:02:46Z

README.md

+
+### Improvements
+- The planner could be implemented with a Builder pattern. An algorithm (Dijktra or A*) receives a type of graph (grid or weighted grid) and a model (small or big). 
+- The rule of five sais that every class requires a user-defined destructor, a user-defined copy constructor, or a user-defined copy assignment operator and move constructor. I did not implement the move and copy constructors, because for this small project, only 1 planner will be used. 


rule of five says

pullrequest · 2019-07-01T19:02:46Z

implementation/include/implementation.h

@@ -0,0 +1,70 @@
+#ifndef __IMPLEMENTATION_H__


Was it a conscious choice to avoid "#pragma once"?

pullrequest · 2019-07-01T19:02:46Z

visualizer/visualizer.cpp

+    bm.bitmapinfoheader.bitsperpixel = bitsPerPixel;
+    bm.bitmapinfoheader.compression = 0;
+    bm.bitmapinfoheader.imagesize = pixelBytes;
+    bm.bitmapinfoheader.ypixelpermeter = 0x130B; //2835 , 72 DPI


I'd suggest lifting 0x130B to a constant

pullrequest · 2019-07-01T19:02:46Z

main.cpp

+};
+
+// Some constants
+enum {


Any way to sort these enums by values lowest to highest?

pullrequest · 2019-07-01T19:02:46Z

visualizer/visualizer.cpp

+}
+
+
+void setColor(std::vector<uint8_t>& vec, size_t i, uint8_t r, uint8_t g, uint8_t b)


Wrap r,g,b in a type, maybe call it RGB.

pullrequest · 2019-07-01T19:02:48Z

visualizer/visualizer.cpp

+        {
+            uint8_t pixelValue = pixelFilter(x, height, *pixels);
+            out.write((const char*)&pixelValue, 1);
+            ++ pixels;


prefix increment is generally a bad idea.

pullrequest · 2019-07-01T19:02:48Z

visualizer/visualizer.cpp

+    size_t startIndex,
+    size_t endIndex,
+    uint8_t startRed,
+    uint8_t startGreen,


using RGB, instead of this function taking 8 args, it can take 4.

pullrequest · 2019-07-01T19:02:48Z

visualizer/visualizer.cpp

+    Bitmap bm;
+
+    // Write header
+    memset(&bm, 0, sizeof(bm));


Add a BitMap constructor that handles all the things below.

pullrequest · 2019-07-01T19:02:48Z

visualizer/include/visualizer.h

+ *        x (from the left), and y (from the top) position of the elevationData. See enum
+ *        ImagePixelValues above for interesting values to return.
+ */
+void writeBMP(


use the Size struct above instead of width height

pullrequest · 2019-07-01T19:02:49Z

visualizer/visualizer.cpp

+    size_t width,
+    size_t height)
+{
+    size_t count = width * height;


can you somehow use std::max?

pullrequest · 2019-07-01T19:02:49Z

CMakeLists.txt

+add_subdirectory(implementation)
+
+add_executable(Bachelor main.cpp)
+target_link_libraries(Bachelor visualizer)


you should do target_link_libraries(Bachelor PUBLIC visualizer)

pullrequest · 2019-07-01T19:02:49Z

implementation/include/implementation.h

+             std::unordered_map<Location, Location> &came_from,
+             std::unordered_map<Location, double> &cost_so_far);
+
+    void findShortestPath(void (*print_fun)(std::pair<int, int>), void (*search_fun)(implementation::Grid *,


instead of void (*searchfun).... I think you can just use one of the above templates.

I'm not too sure about the syntax but something like

void (*search<Grid, implementation::GridLocation>

pullrequest · 2019-07-01T19:02:49Z

CMakeLists.txt

+add_subdirectory(visualizer)
+add_subdirectory(implementation)
+
+add_executable(Bachelor main.cpp)


I'd reach for storing the main.cpp in a SOURCE_FILES var

pullrequest · 2019-07-01T19:02:49Z

implementation/include/planner.h

+        std::ofstream out(filepath);
+        std::stringstream ss;
+
+        for (auto location : result)


pullrequest · 2019-07-01T19:02:49Z

README.md

+
+## Introduction
+
+This project contains two algorithms for planning the quickest path for a rover to pick up a bachelor and carry him to his wedding. Shortest path problems are typically solved bij [Dijkstra](https://en.wikipedia.org/wiki/Dijkstra%27s_algorithm) and it's improved version [A*](https://en.wikipedia.org/wiki/A*_search_algorithm). Follow the instructions below to run find your quickest path. 


typically solved by

pullrequest

Due to inactivity, PullRequest has cancelled this review job. You can reactivate the code review job from the PullRequest dashboard

adding back files to be reviewed

7a5f04b

pullrequest bot reviewed Jul 1, 2019

View reviewed changes

pullrequest bot reviewed Jul 16, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adding back files to be reviewed #1

adding back files to be reviewed #1

tessavdheiden commented Jul 1, 2019

pullrequest bot left a comment

pullrequest bot left a comment

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot Jul 1, 2019

pullrequest bot left a comment


		Moving downwards decreases the path time and upwards increases it. We can model this by changing the time with some X %. However, travelling upwards and downwards, may not be equally as coslty as on flat areas, so upwards should increase the time with some extra percentage.

		Below we see the path planned with a small car (top picture) and a heavy car (bottum picture). The more "weight" the car has, the bigger the difference between going up vs going down for the path time. So a heavy car tries to avoid the hills more than a smaller car.


		void print_coordinate(std::pair<int, int> a);

		template<typename Location, typename Graph>


		<img src="results/dijkstra.png" width="256" height="256" title="Dijkstra">

		A* is a modification of Dijkstra’s Algorithm that is optimized for a single destination. Dijkstra’s Algorithm can find paths to all locations; A* finds paths to one location, or the closest of several locations. It prioritizes paths that seem to be leading closer to a goal. The picture below shows the path planned by A*.

		}


		void setColor(std::vector<uint8_t>& vec, size_t i, uint8_t r, uint8_t g, uint8_t b)


		## Introduction

		This project contains two algorithms for planning the quickest path for a rover to pick up a bachelor and carry him to his wedding. Shortest path problems are typically solved bij [Dijkstra](https://en.wikipedia.org/wiki/Dijkstra%27s_algorithm) and it's improved version [A](https://en.wikipedia.org/wiki/A_search_algorithm). Follow the instructions below to run find your quickest path.

adding back files to be reviewed #1

Are you sure you want to change the base?

adding back files to be reviewed #1

Conversation

tessavdheiden commented Jul 1, 2019

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot Jul 1, 2019

Choose a reason for hiding this comment

pullrequest bot left a comment

Choose a reason for hiding this comment