Freeciv

Dean Brown wrote in #note-4:

Can a tile really be not visible to its owner? Did not think of that, and I'm not sure. Whatever, easy to accommodate - here's better patches.

This forgets the original "TODO" that you were set to fix. The "rumor" system does not work when the borders are disabled.

Borders extend from cities far further than city vision range, so it's common situation that owner does not see some tiles. Unless you have set 'borders' to SEE_INSIDE or EXPAND.

So I think we continue with the original patches (that's what I'm going to push if nobody disagrees)

I prefer the better patches - adds to the existing functionality instead of replacing it. But I'm OK either way.

Dean Brown wrote in #note-7:

I prefer the better patches - adds to the existing functionality instead of replacing it. But I'm OK either way.

Then they need to be updated to address the original TODO issue, or at least the TODO comment should not be removed if it's not addressed.

The "rumor" system does not work when the borders are disabled.

Are you referring to this call in create_barbarian_player()?

  notify_player(NULL, NULL, E_UPRISING, ftc_server,
                _("%s gain a leader by the name %s. Dangerous " 
                  "times may lie ahead."),
                nation_plural_for_player(barbarians),
                player_name(barbarians));

Interpreting that comment is a bit unclear -
1 The comparison to barbarian uprising isn't good because barbarian uprisings get announced to all players regardless of tile or disabled borders.
2 What does "nearby" mean exactly? If "nearby" means adjacent or visible, then tile_is_seen() is true and it gets announced, and the comment is addressed. If it means "up to num tiles away" then I'm not sure if it's a good idea to announce about a tile that is neither owned nor visible. But that's assuming tile_is_seen() is not affected by disabled borders - my testing seems to show that's true.

The "barbarian uprising" reference is outdated. It makes no sense with current barbarians implementation, and should be ignored.

The main point, however, is that with the borders enabled (i.e. when tiles have owners) one gets notifications/rumors about extras (dis)appearing within ones empire. If we keep the "rumor" system, they should be given even with borders disabled, when extra (dis)appearance happens at the area (clearly) controlled by the empire.
Defining what counts as area controlled by the empire isn't necessarily easy. That's why we had only TODO item instead of already implemented code.

At the same time someone might consider whole "rumor" system a misfeature. Maybe user really shouldn't see what happens in tiles that they don't see. That's what your original patches implement.

With the vision based notifications there's slight inconsistency that we don't give notifications of non-spontaneous extra (dis)appearance, i.e., when we see (other player's) Worker finishing a task. But then again, spontaneous events are generally different from player initiated ones.

In my testing with borders disabled the existing code doesn't give any notification because tiles aren't owned. I agree figuring out what counts as area controlled by the empire isn't easy when borders are disabled, and maybe we don't want to go there.
So this patch may be the best we can do for now.

I also found in play testing that when you have an ally giving you shared vision you can get lots of notifications you might prefer not to see, but that can be fixed by going to Options->Messages and turning them off.

Dean Brown wrote in #note-11:

I also found in play testing that when you have an ally giving you shared vision you can get lots of notifications you might prefer not to see

I guess Apollo Program is much worse than shared vision with an ally.

Maybe I implement my own patch proposal for this.

My play testing ended with cultural victory so I didn't get to the space race - yes Apollo would be even worse. Context - this came up because I'm experimenting with implementing storms and volcano eruptions as nuisance extras, which cause damage to adjacent units/cities. If one of those appears next to a unit or city I would want to know about it. Vision based notification solves this, but I agree it's not the best solution. Maybe notify only if there's a unit or city adjacent to the tile - that should be possible (I know how to do it in lua).

I like your proposed fix. Agree I could in theory do my own custom notifications in lua, but it's not easy. There's no event signal for extra appearance (maybe a new feature request?), have to monitor every suitable tile every turn. How slow would it be to check every oceanic tile every turn on a large map?

Another use case I thought of - custom ruleset where a resource extra (e.g. Uranium) only appears when a tech is known (e.g. Atomic Theory), and there is no rmcauses="Disappear". With current code, the tile owner would get notified of appearance (OK), except with BORDERS_DISABLED (maybe not so OK). Your patch would help when BORDERS_DISABLED. Could make an argument that for this use case that all visible (not just owned) tiles get notified, but that requires additional testing for no rmcauses="Disappear".

Just noticed a typo in your comments: ilte -> tile

/* In case of disabled borders, consider owner of the closest
 * city, if within 7 tiles, owner of the ilte. */

There's no event signal for extra appearance (maybe a new feature request?)

Opened #1804, which should be blocked by this one because it touches the same piece of code in srv_main.c routine end_turn().