Skip to main content
added update 2
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

Update

Because the code above still failed for some inputs, I changed the algorithm slightly to separately track rooms and merges. Using that, a new variable std::size_t merges{0}; is added to the top of the routine and instead of --roomcount; above, the line is now ++merges;. The final change is to change the line from return roomcount; to return roomcount-merges;

I am now writing an automated tester to verify this.

Update 2

The automated tester, reworked (and apparently bug free) version of this code and an independently derived flood-fill version are now all written, tested and posted here: Multithreaded testing for counting rooms from a floor plan solution

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

Update

Because the code above still failed for some inputs, I changed the algorithm slightly to separately track rooms and merges. Using that, a new variable std::size_t merges{0}; is added to the top of the routine and instead of --roomcount; above, the line is now ++merges;. The final change is to change the line from return roomcount; to return roomcount-merges;

I am now writing an automated tester to verify this.

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

Update

Because the code above still failed for some inputs, I changed the algorithm slightly to separately track rooms and merges. Using that, a new variable std::size_t merges{0}; is added to the top of the routine and instead of --roomcount; above, the line is now ++merges;. The final change is to change the line from return roomcount; to return roomcount-merges;

I am now writing an automated tester to verify this.

Update 2

The automated tester, reworked (and apparently bug free) version of this code and an independently derived flood-fill version are now all written, tested and posted here: Multithreaded testing for counting rooms from a floor plan solution

added update
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

Update

Because the code above still failed for some inputs, I changed the algorithm slightly to separately track rooms and merges. Using that, a new variable std::size_t merges{0}; is added to the top of the routine and instead of --roomcount; above, the line is now ++merges;. The final change is to change the line from return roomcount; to return roomcount-merges;

I am now writing an automated tester to verify this.

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {

Update

Because the code above still failed for some inputs, I changed the algorithm slightly to separately track rooms and merges. Using that, a new variable std::size_t merges{0}; is added to the top of the routine and instead of --roomcount; above, the line is now ++merges;. The final change is to change the line from return roomcount; to return roomcount-merges;

I am now writing an automated tester to verify this.

Post Made Community Wiki by Edward
Source Link
Edward
  • 67.2k
  • 4
  • 120
  • 284

I found another problem with the code that isn't quite addressed by @Martin R's approach. Here's an example that shows the problem:

7 9
#########
#.#.#.#.#
#.#.#...#
#.#...#.#
#.#.#.#.#
#.......#
#########

The correct answer is 1 room, but with both the original and the suggested change, I get 18446744073709551615 (equivalent to -1 on my 64-bit machine). The corrected version of the first if clause is this:

if (tracker[j]) {
    // also from left?
    if (j && tracker[j-1] && (tracker[j-1] != tracker[j])) {
        auto bigger = tracker[j-1];
        auto smaller = tracker[j];
        // make sure they're in the right order
        if (bigger < smaller) {
            std::swap(smaller, bigger);
        }
        // rooms have joined
        std::replace(tracker.begin(), tracker.end(), bigger, smaller);
        --roomcount;
    }
} else {