Worth porting back to CDDA itself <3

@Zireael07 hi, thanks for the interest in our project. however, as this is BN repository and not DDA repository, could you open a feature request on DDA issues instead? you could mention the PR you want to port via leaving a url on DDA issue, such as:

https://github.com/cataclysmbnteam/Cataclysm-BN/pull/3178

and it would link to this PR automatically.

I know, however many of BN contributors are also DDA contributors and often will open PRs themselves. (Opening a PR necessitates you test the changes, which means you need the ability to compile from source)

Is there an easy way to extract these checks into some function, or closure, that would take x and y and decide for which of the 5 overmaps, and at what point, to call the ter method?

om_dir get_dir(point p) { //Enum declaration skipped
    if (p.x < 0) {
        if (p.y < 0) {
            return DIR_NORTHWEST;
        } else if (p.y > OMAPY - 1) {
            return DIR_SOUTHWEST;
        } else {
            return DIR_WEST;
        }
    } else if (p.x > OMAPX - 1) {
        if (p.y < 0) {
            return DIR_NORTHEAST;
        } else if (p.y > OMAPY - 1) {
            return DIR_SOUTHEAST;
        } else {
            return DIR_EAST;
        }
    } else if (p.y < 0) {
        return DIR_NORTH;
    } else if (p.y > OMAPY - 1) {
        return DIR_SOUTH;
    } else {
        return DIR_HERE;
    }
}

void shift_pos(point p) {
    if (p.x < 0) {
        p.x = OMAPX - 1 + p.x;
    } else if (p.x > OMAPX - 1) {
        p.x = p.x - OMAPX - 1;
    } 
    if (p.y < 0) {
        p.y = OMAPY - 1 + p.y;
    } else if (p.y > OMAPY - 1) {
        p.y = p.y - OMAPY - 1;
    }
}

bool is_corner_needed(overmap o, point p) {
    return o !== nullptr && !is_river_or_lake( o->ter( p ) );
}

...
overmap* om[DIR_MAX] = { nullptr };
om[DIR_NORTHEAST] = nullptr; // Diagonal overmaps lookup *could* be useful, if they'd be already loaded. Since they're not - it doesn't worth the hassle.
om[DIR_NORTH] = north;
om[DIR_HERE] = this;
...
if ( is_corner_needed(om[get_dir(p + point_north_east)], shift_pos(p + point_north_east)) ) {
    ter_set( p, oter_id( "river_c_not_ne" ) );
}

Everything can be refactored. Like to this monstrosity. But it doesn't seems much better to me - two screens of code scattered around where few lines of checks could do the work. Those checks looks ugly, i admit that, but they're trivial and basically same. After spending 5 seconds looking at first one you can tell immediately what all the other ones does. And it's write once code anyway, not something that will need to be tweaked further. (Unless i screwed offsets somewhere ofc, but it won't take a long to notice and fix)
Plus, above solution have worse performance, due to more generic checks, needless functions calls, and other boilerplates. I'd be happy to refactor it to something clean and costless, but i don't see how can it be done - above solution will add 8 extra calls per most cells, having still debatable readability due to needless complexity.

Everything from this line and to the end of the loop should really be extracted into a separate function.

Getting rid of good_river() was a deliberate choice. I don't really trust compiler that it will inline it properly, and won't call it thousands of times, passing increased amount of arguments(overmaps refs), and then extracting x and y with getters, instead of looking at raw loop counter.
I most likely being too paranoid here, but mapgen is already uncomfortable slow, plus new checks of outer overmaps puts additional strain, which i wanted to lessen as much as reasonable possible. I'm almost sure that difference won't be noticeable by any measure, but don't see a reason to leave any chances to compiler to screw.
To my eye reducing indent won't improve readability - there's still no horizontal scroll, no forced line breaks, no astyle ranting, etc. It is wide, but not to the point where it causing discomfort. I'd say it's standing on the edge of the gray zone :) But it if it's important to you, then sure, i can split them back.

There's a lot to debate when it comes to abstractions, code reuse, reliability, readability and performance, but I can agree that it is unlikely anyone will touch this code for years to come, and it's out of the way, so these topics don't matter much for this case or warrant holding up the PR over.

It is indeed very debatable topic. I really don't feel good about deliberate degrading of performance without solid reason. In my world ugly code is better than slow code :) I might be unreasonable too strict about such things, so feel free to insist when you feel it does matter. I'm not stubbornly refusing - just wanted to explain my reasoning, and ask for confirmation - whether i really should do it. Anyway, glad it's settled down.