Skip to main content
added thoughts on efficiency
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

Thoughts on efficiency

Since a delay is part of the program, making the program run faster isn't necessarily a goal, but here are some thoughts on efficiency if you wanted to explore this further. First, I realized belatedly that I hadn't answered your question about the return value for Board::render(). In my view, you have it exactly right in the code now. Returning a reference would be an error because, as soon as the function ends and the ret variable goes out of scope, the destructor is called, rendering a reference invalid. When you return by value as the current code has it, notionally, a copy is created. (I say "notionally" because most compilers are, in fact, smart enough to implement Named Return Value Optimization (NRVO) to avoid actually making a copy.) Also, while you could allocate on the heap and return a pointer, freeing that memory now becomes another problem. For all of these reasons, I'd say that the way you have it is just right.

However, one option for a possible efficiency gain would be for the Board object to contain two copies of the board and simply keep track of which is the current view within nextRound() and render(). That way instead of reallocating a new one (and destroying one) on each call to nextRound, the program could simply use the same two vectors and simply swap them each loop iteration.

Thoughts on efficiency

Since a delay is part of the program, making the program run faster isn't necessarily a goal, but here are some thoughts on efficiency if you wanted to explore this further. First, I realized belatedly that I hadn't answered your question about the return value for Board::render(). In my view, you have it exactly right in the code now. Returning a reference would be an error because, as soon as the function ends and the ret variable goes out of scope, the destructor is called, rendering a reference invalid. When you return by value as the current code has it, notionally, a copy is created. (I say "notionally" because most compilers are, in fact, smart enough to implement Named Return Value Optimization (NRVO) to avoid actually making a copy.) Also, while you could allocate on the heap and return a pointer, freeing that memory now becomes another problem. For all of these reasons, I'd say that the way you have it is just right.

However, one option for a possible efficiency gain would be for the Board object to contain two copies of the board and simply keep track of which is the current view within nextRound() and render(). That way instead of reallocating a new one (and destroying one) on each call to nextRound, the program could simply use the same two vectors and simply swap them each loop iteration.

added not about #define
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

Also the UPSAMPLING constant would be better as a constexpr int rather than a #define. Making that change allows for type checking and costs nothing in terms of runtime performance.

Also the UPSAMPLING constant would be better as a constexpr int rather than a #define. Making that change allows for type checking and costs nothing in terms of runtime performance.

added "fix the bug" section
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

Fix the bug

In the current version of render we have this code:

cv::Mat ret = cv::Mat::zeros(width * UPSAMPLING, height * UPSAMPLING, CV_8UC3);

The problem is that the first two arguments to zeros are rows and columns, so these should be swapped for the code to work correctly for non-square boards. The same reversal is required for the ret.at<> line.

Fix the bug

In the current version of render we have this code:

cv::Mat ret = cv::Mat::zeros(width * UPSAMPLING, height * UPSAMPLING, CV_8UC3);

The problem is that the first two arguments to zeros are rows and columns, so these should be swapped for the code to work correctly for non-square boards. The same reversal is required for the ret.at<> line.

Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284
Loading